899264 - Add OpenGL version support on GLContext

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Guillaume Abadie]

Add a member into GLContext like mVersion = 210 (when it would be a OpenGL 2.1 context)

Comment 1

[Guillaume Abadie]

Attachment: patch step 01 revision 1

I would like to fix this bug in several patches :
 - introducing mVersion and mProfile
 - moving code in GLContext.h, to explicitly say that a given XXX_extension provide a followed functions
 - move extensions management to private, and only use the XXX_extension mechanism that would now also use GLContext::IsAtLeast(Profile::OpenGL, 300) and stuff like that.

This bug patch just introduce : mVersion and mProfile.

Comment 2

[Guillaume Abadie]

Attachment: patch step 02 revision 1

This patch only re-structure GLContext. Moving code in GLContext.h only. Several bug has been found by the same way.
 - GLErrorToString should be static
 - Draw*Instanced don't have BeforeGLDrawCall(); and AfterGLDrawCall();

Comment 3

[Guillaume Abadie]

 - Also, XXX_draw_buffers don't have associated symbols load.
 - In step 01, If add Is*Profil() and corrected by Is*Profile() in step 02

Comment 4

[Guillaume Abadie]

Attachment: patch step 02 revision 2 GLContext.h

Here is how GLContext.h is by applying patch step 1 & 2 revision 1

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 783167 [details] [diff] [review]
patch step 01 revision 1

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

::: gfx/gl/GLContext.h
@@ +1289,5 @@
> +        MOZ_ASSERT(mProfile != Profile::Unknown, "unknown context profile");
> +        MOZ_ASSERT(mVersion != 0, "unknown context version");
> +
> +        if (profile == Profile::OpenGL) {
> +            return profile != Profile::OpenGLES &&

Please do it explicitly like the comment above says, not just checking that we're not GLES.

@@ +1338,5 @@
>      int32_t mRenderer;
>  
> +    inline void SetProfile(Profile profile) {
> +        MOZ_ASSERT(!mInitialized, "SetProfile can only be called before initialization!");
> +        MOZ_ASSERT(profile != Profile::OpenGL && profile != Profile::Unknown, "OpenGL profile not granted");

"Invalid `profile` for SetProfile"

@@ +1344,5 @@
> +    }
> +
> +    inline void SetVersion(uint32_t version) {
> +        MOZ_ASSERT(!mInitialized, "SetVersion can only be called before initialization!");
> +        mVersion = version;

This should assert that `version` is non-zero.

@@ +2743,5 @@
>      }
>  
>  private:
>      void raw_fDepthRange(GLclampf a, GLclampf b) {
> +        MOZ_ASSERT(!IsGLES2());

On second thought, I don't know as this is a great idea. We already have testing in the form of ASSERT_SYMBOL_PRESENT. It's not likely to be true for these funcs, but for others, particularly desktop GL funcs, sometimes GLES will expose desktop GL extensions and their functions, so asserting which GL type each function is usually from removes some flexibility.

@@ +3593,5 @@
>  };
>  
>  uint32_t GetBitsPerTexel(GLenum format, GLenum type);
>  
> +MOZ_INCLASS_FINISH_ENUM_CLASS(GLContext::Profile)

What does this do?

::: gfx/gl/GLContextProviderCGL.mm
@@ +95,5 @@
>            mContext(context),
>            mTempTextureName(0)
> +    {
> +        SetProfile(Profile::OpenGLCompatibility);
> +        SetVersion(210);

Shouldn't we just combine these functions, since we're supposed to set both of them at context creation time?

::: gfx/gl/GLContextProviderGLX.cpp
@@ +966,5 @@
>            mPixmap(aPixmap)
>      {
>          MOZ_ASSERT(mGLX);
> +        SetProfile(Profile::OpenGLCompatibility);
> +        SetVersion(210);

I don't believe this is true. We should be getting anywhere from 2.1, to 3.x for Mesa, to 4.x for binary drivers.

::: gfx/gl/GLContextProviderWGL.cpp
@@ +292,5 @@
>            mLibType(aLibUsed),
>            mIsDoubleBuffered(false)
>      {
> +        SetProfile(Profile::OpenGLCompatibility);
> +        SetVersion(210);

I don't believe this is true either.

::: mfbt/TypedEnum.h
@@ +82,5 @@
>   *   MOZ_END_ENUM_CLASS(Enum)
>   *
> + * you can also define a strongly=typed enumeration feature C++11 ("enum class")
> + * in another class or struct with MOZ_INCLASS_BEGIN_ENUM_CLASS,
> + * MOZ_INCLASS_END_ENUM_CLASS and MOZ_INCLASS_FINISH_ENUM_CLASS. For example,

This change needs to be in a separate patch, in a different bug, filed under MFBT, and reviewed by Waldo, or some other MFBT peer.

Comment 6

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Comment on attachment 783167 [details] [diff] [review]
> patch step 01 revision 1
> 
> Review of attachment 783167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.h
> @@ +1289,5 @@
> > +        MOZ_ASSERT(mProfile != Profile::Unknown, "unknown context profile");
> > +        MOZ_ASSERT(mVersion != 0, "unknown context version");
> > +
> > +        if (profile == Profile::OpenGL) {
> > +            return profile != Profile::OpenGLES &&
> 
> Please do it explicitly like the comment above says, not just checking that
> we're not GLES.
I disagree. This function might be call a lot of time. And MOZ_ASSERT(profile != Profile::OpenGL && profile != Profile::Unknown, "OpenGL profile not granted"); in SetProfile let us optimize by this way without any bug.
> 
> @@ +1338,5 @@
> >      int32_t mRenderer;
> >  
> > +    inline void SetProfile(Profile profile) {
> > +        MOZ_ASSERT(!mInitialized, "SetProfile can only be called before initialization!");
> > +        MOZ_ASSERT(profile != Profile::OpenGL && profile != Profile::Unknown, "OpenGL profile not granted");
> 
> "Invalid `profile` for SetProfile"
Ok.
> 
> @@ +1344,5 @@
> > +    }
> > +
> > +    inline void SetVersion(uint32_t version) {
> > +        MOZ_ASSERT(!mInitialized, "SetVersion can only be called before initialization!");
> > +        mVersion = version;
> 
> This should assert that `version` is non-zero.
Oups ...
> 
> @@ +2743,5 @@
> >      }
> >  
> >  private:
> >      void raw_fDepthRange(GLclampf a, GLclampf b) {
> > +        MOZ_ASSERT(!IsGLES2());
> 
> On second thought, I don't know as this is a great idea. We already have
> testing in the form of ASSERT_SYMBOL_PRESENT. It's not likely to be true for
> these funcs, but for others, particularly desktop GL funcs, sometimes GLES
> will expose desktop GL extensions and their functions, so asserting which GL
> type each function is usually from removes some flexibility.
I've just did a s/mIsGLES2/IsGLES2, that might be changed in another patch ???
> 
> @@ +3593,5 @@
> >  };
> >  
> >  uint32_t GetBitsPerTexel(GLenum format, GLenum type);
> >  
> > +MOZ_INCLASS_FINISH_ENUM_CLASS(GLContext::Profile)
> 
> What does this do?
That let you to avec an enum class defined in a class without C++ error when the compilor doesn't support enum class.
> 
> ::: gfx/gl/GLContextProviderCGL.mm
> @@ +95,5 @@
> >            mContext(context),
> >            mTempTextureName(0)
> > +    {
> > +        SetProfile(Profile::OpenGLCompatibility);
> > +        SetVersion(210);
> 
> Shouldn't we just combine these functions, since we're supposed to set both
> of them at context creation time?
For this patch, there are set at the lowest known value. I was planing to do an other patch to add a string parser of GL_VERSION. And I want to do this parser in a different patch, because it will have to be very strong to prevent any fancy GL_VERSION string given by the driver.
> 
> ::: gfx/gl/GLContextProviderGLX.cpp
> @@ +966,5 @@
> >            mPixmap(aPixmap)
> >      {
> >          MOZ_ASSERT(mGLX);
> > +        SetProfile(Profile::OpenGLCompatibility);
> > +        SetVersion(210);
> 
> I don't believe this is true. We should be getting anywhere from 2.1, to 3.x
> for Mesa, to 4.x for binary drivers.
It's true, but we have the chance to not use deprecated features, so that is still working. I already tried on push to try. =) That is plane to be changed with the GL_VERSION parser.
> 
> ::: gfx/gl/GLContextProviderWGL.cpp
> @@ +292,5 @@
> >            mLibType(aLibUsed),
> >            mIsDoubleBuffered(false)
> >      {
> > +        SetProfile(Profile::OpenGLCompatibility);
> > +        SetVersion(210);
> 
> I don't believe this is true either.
Same.
> 
> ::: mfbt/TypedEnum.h
> @@ +82,5 @@
> >   *   MOZ_END_ENUM_CLASS(Enum)
> >   *
> > + * you can also define a strongly=typed enumeration feature C++11 ("enum class")
> > + * in another class or struct with MOZ_INCLASS_BEGIN_ENUM_CLASS,
> > + * MOZ_INCLASS_END_ENUM_CLASS and MOZ_INCLASS_FINISH_ENUM_CLASS. For example,
> 
> This change needs to be in a separate patch, in a different bug, filed under
> MFBT, and reviewed by Waldo, or some other MFBT peer.
Ok.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 783327 [details] [diff] [review]
patch step 02 revision 1

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

You really should have done this first, as any churn (including changes to ancestor patches!) in this file will make huge rejections for you.

It should be possible to split this into smaller moves. With moving large parts of the file at once, the diff creation algo tends to fall down, like it does here. It would be much better if you could split some of these moves into other patches, so it's more clear that we're just moving bits and pieces. (Move the enums, then consolidate the extension stuff, etc)

I largely like how things are consolidated, I think. Let's see what bjacob thinks.

::: gfx/gl/GLContext.h
@@ +156,5 @@
> +
> +    /**
> +     * Return true if we are running on a OpenGL core profil context
> +     */
> +    inline bool IsCoreProfile() const {

`inline` is already implied when we define functions in the body of the class.

@@ +203,5 @@
> +        MOZ_ASSERT(mVersion != 0, "unknown context version");
> +
> +        if (profile == Profile::OpenGL) {
> +            return profile != Profile::OpenGLES &&
> +            version >= mVersion;

indents

@@ +207,5 @@
> +            version >= mVersion;
> +        }
> +
> +        return profile == mProfile &&
> +        version >= mVersion;

indents

@@ +444,5 @@
> +    ExtensionBitset<Extensions_Max> mAvailableExtensions;
> +
> +
> +// -----------------------------------------------------------------------------
> +// XXX_* Package manager

Indent these to match the current scope.

@@ +456,3 @@
>       * This enum should be sorted by name.
>       */
>      enum GLExtensionPackages {

Why is this not with the rest of the enums?

@@ +520,5 @@
>      }
>  
> +
> +// -----------------------------------------------------------------------------
> +// Deprecated package manager (use XXX_* package mechanism instead)

s/package manager/extension group queries/

@@ +537,5 @@
> +    }
> +
> +
> +// -----------------------------------------------------------------------------
> +// Deprecated robustness manager (would like to move to XXX_robustness)

I don't know if we can say this yet, so let's not. IIRC, this was more complicated than simply an extension query.

@@ +2885,5 @@
> +     * Context reset constants.
> +     * These are used to determine who is guilty when a context reset
> +     * happens.
> +     */
> +    enum ContextResetARB {

Why is this way down here?

Comment 8

[Kelsey Gilbert [:jgilbert]]

(In reply to Guillaume Abadie from comment #6)
> (In reply to Jeff Gilbert [:jgilbert] from comment #5)
> > Comment on attachment 783167 [details] [diff] [review]
> > patch step 01 revision 1
> > 
> > Review of attachment 783167 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/gl/GLContext.h
> > @@ +1289,5 @@
> > > +        MOZ_ASSERT(mProfile != Profile::Unknown, "unknown context profile");
> > > +        MOZ_ASSERT(mVersion != 0, "unknown context version");
> > > +
> > > +        if (profile == Profile::OpenGL) {
> > > +            return profile != Profile::OpenGLES &&
> > 
> > Please do it explicitly like the comment above says, not just checking that
> > we're not GLES.
> I disagree. This function might be call a lot of time. And
> MOZ_ASSERT(profile != Profile::OpenGL && profile != Profile::Unknown,
> "OpenGL profile not granted"); in SetProfile let us optimize by this way
> without any bug.
I'm not saying it's not currently correct, but rather we should make it robust such that it's more likely to survive changes. If we added a GLESCompat profile option, it's not as obvious that we need to change how *Profile::OpenGL* is handled. If we make the handling explicit, we will only need to change it when we make changes to Profile::OpenGL, instead of any other part of the enum.

If a function like this is showing up in profiles (I promise you, it's not), the correct response is to cache the result, not just halve the number of branches.
> > @@ +2743,5 @@
> > >      }
> > >  
> > >  private:
> > >      void raw_fDepthRange(GLclampf a, GLclampf b) {
> > > +        MOZ_ASSERT(!IsGLES2());
> > 
> > On second thought, I don't know as this is a great idea. We already have
> > testing in the form of ASSERT_SYMBOL_PRESENT. It's not likely to be true for
> > these funcs, but for others, particularly desktop GL funcs, sometimes GLES
> > will expose desktop GL extensions and their functions, so asserting which GL
> > type each function is usually from removes some flexibility.
> I've just did a s/mIsGLES2/IsGLES2, that might be changed in another patch
> ???
Yep, this is there for other reasons. That's what I get for mostly looking at the right side of the diff!
> > 
> > @@ +3593,5 @@
> > >  };
> > >  
> > >  uint32_t GetBitsPerTexel(GLenum format, GLenum type);
> > >  
> > > +MOZ_INCLASS_FINISH_ENUM_CLASS(GLContext::Profile)
> > 
> > What does this do?
> That let you to avec an enum class defined in a class without C++ error when
> the compilor doesn't support enum class.
'avec'? :)
> > 
> > ::: gfx/gl/GLContextProviderCGL.mm
> > @@ +95,5 @@
> > >            mContext(context),
> > >            mTempTextureName(0)
> > > +    {
> > > +        SetProfile(Profile::OpenGLCompatibility);
> > > +        SetVersion(210);
> > 
> > Shouldn't we just combine these functions, since we're supposed to set both
> > of them at context creation time?
> For this patch, there are set at the lowest known value. I was planing to do
> an other patch to add a string parser of GL_VERSION. And I want to do this
> parser in a different patch, because it will have to be very strong to
> prevent any fancy GL_VERSION string given by the driver.
That wasn't the question I was asking here:
Why not have one function: SetProfileAndVersion(Prof::GLCompat, 210)?
> > 
> > ::: gfx/gl/GLContextProviderGLX.cpp
> > @@ +966,5 @@
> > >            mPixmap(aPixmap)
> > >      {
> > >          MOZ_ASSERT(mGLX);
> > > +        SetProfile(Profile::OpenGLCompatibility);
> > > +        SetVersion(210);
> > 
> > I don't believe this is true. We should be getting anywhere from 2.1, to 3.x
> > for Mesa, to 4.x for binary drivers.
> It's true, but we have the chance to not use deprecated features, so that is
> still working. I already tried on push to try. =) That is plane to be
> changed with the GL_VERSION parser.
File a bug, and make a comment here. We can't just add code which is wrong-ish without giving context, or explaining why it's correct enough to live with, especially if it's only supposed to be temporary. Sometimes temporary changes end up becoming semi-permanent via forgetfulness or accidentally constructing dependencies against them.

Comment 9

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> Comment on attachment 783327 [details] [diff] [review]
> patch step 02 revision 1
> 
> Review of attachment 783327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You really should have done this first, as any churn (including changes to
> ancestor patches!) in this file will make huge rejections for you.
Yeah you right, I will handle that, going to swicth step 1 and step 2.
> 
> It should be possible to split this into smaller moves. With moving large
> parts of the file at once, the diff creation algo tends to fall down, like
> it does here. It would be much better if you could split some of these moves
> into other patches, so it's more clear that we're just moving bits and
> pieces. (Move the enums, then consolidate the extension stuff, etc)
> 
> I largely like how things are consolidated, I think. Let's see what bjacob
> thinks.
> 
> ::: gfx/gl/GLContext.h
> @@ +156,5 @@
> > +
> > +    /**
> > +     * Return true if we are running on a OpenGL core profil context
> > +     */
> > +    inline bool IsCoreProfile() const {
> 
> `inline` is already implied when we define functions in the body of the
> class.
Oups ...
> 
> @@ +203,5 @@
> > +        MOZ_ASSERT(mVersion != 0, "unknown context version");
> > +
> > +        if (profile == Profile::OpenGL) {
> > +            return profile != Profile::OpenGLES &&
> > +            version >= mVersion;
> 
> indents
Move artefacts
> 
> @@ +207,5 @@
> > +            version >= mVersion;
> > +        }
> > +
> > +        return profile == mProfile &&
> > +        version >= mVersion;
> 
> indents
Move artefacts
> 
> @@ +444,5 @@
> > +    ExtensionBitset<Extensions_Max> mAvailableExtensions;
> > +
> > +
> > +// -----------------------------------------------------------------------------
> > +// XXX_* Package manager
> 
> Indent these to match the current scope.
> 
> @@ +456,3 @@
> >       * This enum should be sorted by name.
> >       */
> >      enum GLExtensionPackages {
> 
> Why is this not with the rest of the enums?
Because I want to keep all things related to this new feature at the same place in the code.
> 
> @@ +520,5 @@
> >      }
> >  
> > +
> > +// -----------------------------------------------------------------------------
> > +// Deprecated package manager (use XXX_* package mechanism instead)
> 
> s/package manager/extension group queries/
Ok.
> 
> @@ +537,5 @@
> > +    }
> > +
> > +
> > +// -----------------------------------------------------------------------------
> > +// Deprecated robustness manager (would like to move to XXX_robustness)
> 
> I don't know if we can say this yet, so let's not. IIRC, this was more
> complicated than simply an extension query.
Yeah I've seen that, but my mind is that would be more user friendly to fake like it would be a XXX_robustness.
> 
> @@ +2885,5 @@
> > +     * Context reset constants.
> > +     * These are used to determine who is guilty when a context reset
> > +     * happens.
> > +     */
> > +    enum ContextResetARB {
> 
> Why is this way down here?

Comment 10

[Guillaume Abadie]

Ok, I'm going to do GLContext.h stuff in a another bug blocking this one. That will be easier.

Comment 11

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

Comment on attachment 783327 [details] [diff] [review]
patch step 02 revision 1

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

Looked at this with Guillaume; this looks like some useful reorganization... useful enough to justify the annoyance of a more complicated hg history.

Comment 12

[Guillaume Abadie]

Attachment: patch revision 2

Comment 13

[Guillaume Abadie]

Attachment: patch revision 3

Add uint32_t Version{Major,Minor}(); and GetProfileName for future use.

Comment 14

[Kelsey Gilbert [:jgilbert]]

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

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

This is fine, though I don't think we need some of the functions you added here. Remove those and r+.

::: gfx/gl/GLContext.h
@@ +149,5 @@
>          return false;
>      }
>  
> +    /**
> +     * Return true if we are running on a OpenGL core profil context

s/profil/profile/ here, and in a number of places below.

@@ +231,5 @@
> +    inline uint32_t VersionMajor() const {
> +        return mVersion / 100;
> +    }
> +
> +    inline uint32_t VersionMinor() const {

I don't think VersionMajor or VersionMinor are useful, given the functionality provided by IsAtLeast, and the fact that they are useless without also checking the context's profile. We should just use IsAtLeast anywhere where we might want these funcs.

@@ +285,5 @@
>      bool mIsOffscreen;
>      bool mIsGlobalSharedContext;
>      bool mContextLost;
> +
> +    uint32_t mVersion;

We should probably just use 'int' or 'unsigned int' for this, since we hardly need all 32 bits.

::: gfx/gl/GLContextProviderCGL.mm
@@ +94,5 @@
>          : GLContext(caps, shareContext, isOffscreen),
>            mContext(context),
>            mTempTextureName(0)
> +    {
> +        SetProfileVersion(ContextProfile::OpenGLCompatibility, 210);

Are we sure these defaults should be 210 and not 200?

Comment 15

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Comment on attachment 784615 [details] [diff] [review]
> patch revision 3
> 
> Review of attachment 784615 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is fine, though I don't think we need some of the functions you added
> here. Remove those and r+.
> 
> ::: gfx/gl/GLContext.h
> @@ +149,5 @@
> >          return false;
> >      }
> >  
> > +    /**
> > +     * Return true if we are running on a OpenGL core profil context
> 
> s/profil/profile/ here, and in a number of places below.
Oups...
> 
> @@ +231,5 @@
> > +    inline uint32_t VersionMajor() const {
> > +        return mVersion / 100;
> > +    }
> > +
> > +    inline uint32_t VersionMinor() const {
> 
> I don't think VersionMajor or VersionMinor are useful, given the
> functionality provided by IsAtLeast, and the fact that they are useless
> without also checking the context's profile. We should just use IsAtLeast
> anywhere where we might want these funcs.
Ok.
> 
> @@ +285,5 @@
> >      bool mIsOffscreen;
> >      bool mIsGlobalSharedContext;
> >      bool mContextLost;
> > +
> > +    uint32_t mVersion;
> 
> We should probably just use 'int' or 'unsigned int' for this, since we
> hardly need all 32 bits.
Replaced by an unsigned int
> 
> ::: gfx/gl/GLContextProviderCGL.mm
> @@ +94,5 @@
> >          : GLContext(caps, shareContext, isOffscreen),
> >            mContext(context),
> >            mTempTextureName(0)
> > +    {
> > +        SetProfileVersion(ContextProfile::OpenGLCompatibility, 210);
> 
> Are we sure these defaults should be 210 and not 200?
Fixed.

Comment 16

[Guillaume Abadie]

Attachment: patch revision 4

Comment 17

[Guillaume Abadie]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab3c575381b2

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ab3c575381b2