971914 - Implement OGL State tracking

Status: NEW

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Currently we have no state tracking or state conventions. Tiling with OGL will make this worse. We can mitigate this before tiling is ready and likely get some general wins before tiling.

Bas tells me we don't really need this for D3D so an OGL specific is fine.

Comment 1

[Benoit Girard (:BenWa)]

Are you interested in taking this matt?

Comment 2

[Matt Woodrow (:mattwoodrow)]

I could be, yeah.

Adding ni?bjacob for information about how this might look (assuming we want to implement it on or near GLContext). I know he's actively trying to trim GLContext, so I don't want to go ahead with adding more stuff without checking.

Comment 3

[Benoit Girard (:BenWa)]

Here's what I envision:
We have something that tracks the GLContext state. We must be able to reset all tracke state to 'I don't know' when the context is touched by 3rd party code.

On particular calls like draw calls we need to have something to diff the GLState with the request state and make GL calls for any mismatch. This will let us have redundant program and texture state changes without information GL about it.

I can think of two ways to implement this: Either have it live almost internally in GLContext or instead we can have an external state object that we call .bind() on. This would be easier to support both OGL and D3D but Bas tells me that we shouldn't worry about supporting D3D. So perhaps having this internal to GLContext is the right thing to do.

Comment 4

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

Indeed, you're at a crossroads here, as comment 3 explains: you can either (option #1) implement state tracking either behind the OpenGL API or (option #2) at a higher level that might then also map to D3D. I don't have an opinion as to what's best, but option #1 is a lot more straightforward while option #2 would require much thinking to get it right.

If you do option #1, as you say above, you could then do it directly in GLContext. I really appreciate that comment 2 is careful about not re-cluttering GLContext after we spent much effort trimming it (bug 942491, which is finished now). That said, the current design of GLContext doesn't really leave any alternative to building such state tracking into GLContext itself. The design of GLContext doesn't lend itself easily to adding such features as state tracking, that affect the behavior of many GL entry points, in a way that is self-contained.

But state tracking (contrary to the many extraneous features that GLContext used to have, that got moved out of it in bug 942491) is a fairly reasonable thing to build directly in GLContext, at least as a first step. And we already have some of it: see e.g. fGetIntegerv. So I really think that for this particular project, you should feel free to add your code directly in GLContext --- at least as a first step.

There is a longer-term project to change the design of GLContext to one that would allow adding such features, that change the behavior of many GL entry points, without having to hardcode all these features in the central GLContext files. That's bug 901498. State tracking has always been one of the envisioned applications for that.

To summarize I think that you should do what is most convenient/efficient for your immediate needs, including adding code to GLContext as you need to, and then hopefully we'll someday get to bug 901498. Having your state tracking already in GLContext might even help keep discussion around bug 901498 more concrete.

Comment 5

[Andreas Gal :gal]

Can we implement a wrapper that has the same interface as GLContext and takes a GLContext and tracks the state around it and calls into the passed in real GLContext?

Comment 6

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

Yes, that is one of the wrapper types considered in bug 901498. However:

 1. Nothing prevents us from first implementing state tracking directly in GLContext at first, and later getting to bug 901498 to make it a wrapper.

 2. There is some preparatory work needed before we can do bug 901498 right. For example, the approach proposed in bug 901498 would add an overhead of (N+1) virtual function calls to each GL call, where N is the number of wrappers around the 'raw' GLContext. Before we start implementing that, we need to do some measurements: how many GL calls do we need to make per second, and how much overhead would that add? I don't know at present. If that turns out to be significant overhead, we would then consider more lightweight alternatives, etc.

 3. Since state tracking is a good candidate to be moved to a wrapper, having it already implemented would provide a useful, concrete experimentation ground to implement the wrappers mechanism.

That's why in comment 4 I was saying that it's OK to just implement the state tracking directly in GLContext, for now.

Comment 7

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

Note that once we have a state tracker for a GLContext, we probably need to make it mandatory to use permanently for all usage of that GLContext. The alternative would be to have methods to "enter" and "leave" state-tracking, but then the "enter" method would have to make many GL getter calls, which would re-introduce overhead; I don't say that that overhead would be intolerable, but it would sort of defeat the point of having state-tracking to reduce GL overhead. Of course, if we had very long sections between "enter" and "leave", that could still be interesting, but in practice, I believe that a permanent state tracking associated with a GLContext will better match our usage pattern of OpenGL.

Comment 8

[Andreas Gal :gal]

There is no need to use virtual methods and a true interface here. We can implement a simple LayerGLContext class that is casually call compatible for the small subset of GL calls CompositorOGL does and it tracks the state of those and passes them at each draw call to the actual GL context.

Comment 9

[Matt Woodrow (:mattwoodrow)]

I think a simple wrapper class probably is a good way to go, so we can keep this isolated.

I also think rather than waiting until a draw call, we should just make the GL changes immediately.

This protects us against:

gl()->fBindTexture(1);
gl()->fBindTexture(1);

But not against:

gl()->fBindTexture(1);
gl()->fBindTexture(2);
gl()->fBindTexture(1);

I think that's ok, because the latter is just dumb and we should stop doing it.

It also massively simplifies the amount of state we need to track, since if any texture state was modified while '2' was bound we still need to make sure that gets updated.

If we need to cache texture state, then we should add c++ wrapper object around texture that track this instead. This is what we do for shader programs.


We're also unnecessarily good citizens in a lot of our code, reverting all the GLContext state that we change.

e.g: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/TextureHostOGL.cpp#1084

If we try bind 3 of these textures (for YUV) to TEXTURE0, TEXTURE1, TEXTURE2 respectively then we'll get calls like:

glActiveTexture(GL_TEXTURE0)
glActiveTexture(GL_TEXTURE0)
glActiveTexture(GL_TEXTURE1)
glActiveTexture(GL_TEXTURE0)
glActiveTexture(GL_TEXTURE2)
glActiveTexture(GL_TEXTURE0)

If we stop resetting the state, then we skip a lot of these unnecessary calls. We may need to instead make sure that we always set things, instead of relying on them to be unchanged from earlier. The GLContext cache should get rid of most of those though.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Add new GL macros to print function parameters

We'll need to go through an start using these macros for all the GL functions, but once done it should make finding unnecessary GL work a lot easier.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Restore less state

Pretty sure this was all unnecessary work.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache the value of glActiveTexture

This is more or less what I'm planning to do (unless someone disagrees) for all the functions we think will be useful.

I'm sure we can improve performance/memory usage by using a single variable instead of two, and/or using bitfields to define which state is currently cached.

Comment 13

[Andreas Gal :gal]

Comment on attachment 8376919 [details] [diff] [review]
Cache the value of glActiveTexture

This doesn't catch cases where we reset the state and then re-set it.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache the value of glActiveTexture v2

Actually, I think this is better.

Comment 15

[u480271]

Apologies for jumping in out of the blue, but in December when :BenWa showed me the GL calls on OSX, I saw a lot of redundant state changes. I saw the same when I looked at a sample emscripten shadow mapping sample.

I was thinking that we should implement something like the OGL state filtering in backend of the Quake 3 renderer. It looks pretty simple and might be a source of inspiration.

Comment 16

[Matt Woodrow (:mattwoodrow)]

(In reply to Dan Glastonbury :djg :kamidphish from comment #15)
> Apologies for jumping in out of the blue, but in December when :BenWa showed
> me the GL calls on OSX, I saw a lot of redundant state changes. I saw the
> same when I looked at a sample emscripten shadow mapping sample.
> 
> I was thinking that we should implement something like the OGL state
> filtering in backend of the Quake 3 renderer. It looks pretty simple and
> might be a source of inspiration.

Do you have a link for this?

Comment 17

[Matt Woodrow (:mattwoodrow)]

With my current patch queue (needs lots of clean up), drawing a tile now only has

2 glUniformMatrix4fv calls (layer quad rect, and layer transform)
1 glBindTexture
1 glDrawArrays

I think that's the best we can do, unless we try batching multiple tile texture draws into a single glDrawArrays.

I guess we could also combine the two matrices on the CPU and only have one glUniformMatrix4fv call, but I doubt that would be a win.

Comment 18

[Andreas Gal :gal]

This is great and as good as it will get.

The only way I could see tiling improve is by allocating larger gralloc bufers that hold an atlas of tiles that we then can render with a single draw call with a more complex geometry.

The downside is that we would have to do more copying when updating tiles (and we need to double buffer every tile).

Comment 19

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

Another downside might be more locking contention, I suppose.

Comment 20

[Matt Woodrow (:mattwoodrow)]

It looks like (testing locally), that this is about a 5-10% tscroll win, with tiling enabled.

I don't think we're overly composition bound in those tests though, so this could be quite a big win.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Playing around with the compositor running as fast as it can.

There doesn't seem to be any FPS win with these patches, sits around 200fps either way..

Looks like we spend a lot less time in glDrawArrays (~2μs per call instead of ~8μs), but instead end up spending longer in CGLFlushDrawable (4377μs vs 3250μs).

Comment 22

[Benoit Girard (:BenWa)]

Your comment 20/21 seems to contradict or maybe I'm misunderstanding. I wouldn't expect to see a 5-10% improvements if we're not composition bound? Esp. if you say below you don't see significant improvements in the draw times.

In any case I don't think it's worth spending a lot of engineering hours investigating this too closely IMO. Some drivers may not be hurt by this but I'm sure some drivers handle some state change poorly and it's easy and cheap for us to do it ourselves.

Comment 23

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

(In reply to Benoit Jacob [:bjacob] from comment #19)
> Another downside might be more locking contention, I suppose.

...though on second thought, such lock contention, between a client process writing to a gralloc buffer and the compositor reading from it, should only happen in the single-buffering case.

Comment 24

[Matt Woodrow (:mattwoodrow)]

(In reply to Benoit Girard (:BenWa) from comment #22)
> Your comment 20/21 seems to contradict or maybe I'm misunderstanding. I
> wouldn't expect to see a 5-10% improvements if we're not composition bound?
> Esp. if you say below you don't see significant improvements in the draw
> times.
> 
> In any case I don't think it's worth spending a lot of engineering hours
> investigating this too closely IMO. Some drivers may not be hurt by this but
> I'm sure some drivers handle some state change poorly and it's easy and
> cheap for us to do it ourselves.

That is correct, it is indeed a contradiction. I don't have an explanation for it either.

Comment 25

[Matt Woodrow (:mattwoodrow)]

Attachment: Add new GL macros to print function parameters v2

This makes the output from MOZ_GL_DEBUG_VERBOSE a lot more useful when trying to spot unnecessary state changes.

Comment 26

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't set and restore state constantly inside CompositorOGL

Rather than doing a get everytime we want to change everything so we can restore it later, let's just write the state that we need.

Comment 27

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache the value of glActiveTexture

Comment 28

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache the value of glBindTexture

Comment 29

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache the value of glUseProgram

Comment 30

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache the value of glBindBuffer

Comment 31

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache the value of glVertexAttribArray

This should cover all the GL calls we make per-quad when drawing with the compositor.

Comment 32

[Matt Woodrow (:mattwoodrow)]

Attachment: Make TextureSourceOGL responsible for tracking filter state

Instead of setting the filter each time, lets make the owner of the texture responsible (or at least able to be responsible) for knowing what the current filter is.

This is very similar to what we do for shader programs, and is much simpler than trying to have GLContext keep track of all textures and what filters they have set.

Comment 33

[Kelsey Gilbert [:jgilbert]]

I would really appreciate if this stuff were define-disableable, so it's possible to check it for issues in the future.

Comment 34

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8384974 [details] [diff] [review]
Cache the value of glBindTexture

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

Yay for finally doing this caching.

::: gfx/gl/GLContext.h
@@ +564,5 @@
>      /*** In GL debug mode, we completely override glGetError ***/
>  
>      GLenum fGetError()
>      {
> +        mCachedState.Clear();

Why is this done?

@@ +737,5 @@
>  // GL official entry points
>  public:
>  
>      void fActiveTexture(GLenum texture) {
> +        if (mCachedState.mActiveTexture == texture) {

Couldn't we just set up the API such that the meme was:
void fFoo(bar) {
  if (mCachedState.Foo(bar)) {
    //skipped
    return
  }
  fFoo(bar)
}

This way none of the details of mCachedState get leaked into the GLContext funcs.

@@ +2083,5 @@
> +        if (n == 1) {
> +            for (size_t i = 0; i < 2; i++) {
> +                for (size_t j = 0; j < 3; j++) {
> +                    if (mCachedState.mBindTexture[i][j] == *names) {
> +                        mCachedState.mBindTexture[i][j] = 0;

Keep this complexity in CachedState. Consider CachedState::DeleteTextures(n, names). Also, this stuff should go in fDeleteTextures not raw_fDeleteTextures.

@@ +2971,5 @@
> +                target != LOCAL_GL_TEXTURE_RECTANGLE_ARB) {
> +                return nullptr;
> +            }
> +
> +            static_assert(LOCAL_GL_TEXTURE2 - LOCAL_GL_TEXTURE0 == 2, "Must be sequential!");

Not really needed. This is Just True for OGL, same as for COLOR_ATTACHMENTi and cubemap image enum ranges.

@@ +2973,5 @@
> +            }
> +
> +            static_assert(LOCAL_GL_TEXTURE2 - LOCAL_GL_TEXTURE0 == 2, "Must be sequential!");
> +            if (mActiveTexture < LOCAL_GL_TEXTURE0 ||
> +                mActiveTexture > LOCAL_GL_TEXTURE) {

You missed a '2'.

@@ +2981,5 @@
> +            uint32_t index = mActiveTexture - LOCAL_GL_TEXTURE0;
> +            return &mBindTexture[target == LOCAL_GL_TEXTURE_2D ? 0 : 1][index];
> +        }
> +
> +        GLuint mBindTexture[2][3];

It seems like you could do this much cleaner with switch statements rather than packing it into this 2d array.

Comment 35

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

Comment on attachment 8384969 [details] [diff] [review]
Add new GL macros to print function parameters v2

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

R+ with an optional suggestion:

::: gfx/gl/GLContext.h
@@ +761,5 @@
>          AFTER_GL_CALL;
>      }
>  
>      void fBindBuffer(GLenum target, GLuint buffer) {
> +        BEFORE_GL_CALL_PRINT("%i, %i", target, buffer);

Don't you want to print GLenums as 0x%x ?

As a side note, printf format characters may not be the best tool for your needs here, as it forces you to manually specify format characters everytime (whereas they could be mostly determined from the actual types you're passing) and it can't give you features that you could reasonably want in the future (like printing out GLenum names like TEXTURE_2D instead of numeric values, like APItrace does)

I would have the macro here not take printf format characters, and instead forward its argument list to a template function that would be templated in each of its arguments' types (so it would implicitly detect argument types) (and maybe overload that function for each possible number of argument, until we can have variadic templates everywhere), and then use helpers to print each argument, specialized for each type. You would run into the issue that GLenum is just the same type as GLuint, which you could work around by wrapping GLenums in a helper struct when passing them: BEFORE_GL_CALL_PRINT(WrapEnum(target), buffer);

Comment 36

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

Comment on attachment 8384973 [details] [diff] [review]
Cache the value of glActiveTexture

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

r+ but either remove the public ClearCachedState method or justify it.

::: gfx/gl/GLContext.h
@@ +741,5 @@
> +            BEFORE_GL_CALL_SKIPPED("%i", texture);
> +            return;
> +        }
> +        mCachedState.mCachedActiveTexture = texture;
> +        BEFORE_GL_CALL_PRINT("%i", texture);

It's a little bit unfortunate that the BEFORE_GL_CALL_SKIPPED("%i", texture); and BEFORE_GL_CALL_PRINT("%i", texture); have to duplicate each other. I wonder if a slightly different design could avoid that, though I can't find a great one at the moment.

@@ +2540,5 @@
>  
> +    void ClearCachedState()
> +    {
> +        mCachedState.Clear();
> +    }

I am unsure why such a public method is needed. Should the outside world be concerned at all with such an internal detail?

@@ +2938,5 @@
> +    // GL state cache
> +    struct CachedState {
> +      void Clear() {
> +        memset(this, 0, sizeof(CachedState));
> +      }

If this struct has such a Clear() method, don't you want to have its constructor call it?

Comment 37

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

Comment on attachment 8384974 [details] [diff] [review]
Cache the value of glBindTexture

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

Jeff had some good comments here, which already mean this needs a second round. Another comment on memset below

I'll reply just below to one of Jeff's comments:

::: gfx/gl/GLContext.h
@@ +2962,5 @@
> +            memset(this, 0, sizeof(CachedState));
> +        }
> +
> +        void ClearCachedTextures() {
> +            memset(&mBindTexture[0][0], 0, sizeof(GLuint) * 6);

The memset usage is getting too much here. I would much rather manually zero members, and in the present case, just call .Clear() on an array.

@@ +2981,5 @@
> +            uint32_t index = mActiveTexture - LOCAL_GL_TEXTURE0;
> +            return &mBindTexture[target == LOCAL_GL_TEXTURE_2D ? 0 : 1][index];
> +        }
> +
> +        GLuint mBindTexture[2][3];

If this needs to be an array, then make it a nsTAutoArray so you can get memory safety and convenient methods such as Clear().

Comment 38

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

Comment on attachment 8384975 [details] [diff] [review]
Cache the value of glUseProgram

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

::: gfx/gl/GLContext.h
@@ +2990,5 @@
>          }
>  
>          GLuint mBindTexture[2][3];
> +        bool mHasProgram;
> +        GLuint mProgram;

Remove mHasProgram, it is redundant state with mProgram. Indeed, mHasProgram is just !!mProgram. See the OpenGL ES 2.0.25 spec, 2.10.6 Required State, page 43:

http://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf

"Additionally, one unsigned integer is required to hold the name of the current program object. Initially the current program object is invalid, as if UseProgram had been called with program set to zero."

Comment 39

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

Comment on attachment 8384976 [details] [diff] [review]
Cache the value of glBindBuffer

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

::: gfx/gl/GLContext.h
@@ +3008,5 @@
>          bool mHasProgram;
>          GLuint mProgram;
>          GLenum mActiveTexture;
> +        GLuint mArrayBuffer;
> +        bool mHasArrayBuffer;

Same comment as on the previous patch: mHasArrayBuffer is redundant as just !!mArrayBuffer. This is in the GLES 2.0.25 spec, http://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf , Section 2.9 Buffer Objects, page 23: "In the initial state the reserved name zero is bound to ARRAY_BUFFER. There is no buffer object corresponding to the name zero [...] "

Also, btw, mArrayBuffer would be more explicit if renamed to mBoundArrayBuffer.

Comment 40

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

Comment on attachment 8384977 [details] [diff] [review]
Cache the value of glVertexAttribArray

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

::: gfx/gl/GLContext.h
@@ +1011,5 @@
>          AFTER_GL_CALL;
>      }
>  
>      void fDisableVertexAttribArray(GLuint index) {
> +        if (index < 2) {

These literal '2' should not be repeated in multiple places; instead, they should read this value 2 off a constant that can be adjusted by changing only 1 line of code.

@@ +3041,5 @@
> +          mVertexAttribs[0].ClearAttribs();
> +          mVertexAttribs[1].ClearAttribs();
> +        }
> +
> +        struct VertexAttribs {

This struct wanted to be named VertexAttrib without the 's', right?

@@ +3064,5 @@
> +            void ClearAttribs()
> +            {
> +              mSize = 0;
> +              mStride = 0;
> +              mPointer = 0;

Why not clear all?

@@ +3068,5 @@
> +              mPointer = 0;
> +            }
> +
> +            bool mEnabledKnown;
> +            bool mEnabled;

EnabledKnown and Enabled sound like they wanted to be a 3-state enum, not a pair of two bools (when EnabledKnown is false, Enabled is meaningless, right?). This would allow to simplify the above code.

Comment 41

[Kelsey Gilbert [:jgilbert]]

I'm going to echo that GLenum should be printed as %x, not %d. This is also true for GLbitfield. Pointer types will naturally be %p.

Comment 42

[Matt Woodrow (:mattwoodrow)]

Attachment: Add new GL macros to print function parameters

Comment 43

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache the value of glActiveTexture

Comment 44

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache the value of glBindTexture

Comment 45

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache the value of glUseProgram

Comment 46

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache the value of glBindBuffer

Comment 47

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache the value of glVertexAttribArray

Comment 48

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Gilbert [:jgilbert] from comment #34)
> Comment on attachment 8384974 [details] [diff] [review]
> Cache the value of glBindTexture
> 
> Review of attachment 8384974 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yay for finally doing this caching.
> 
> ::: gfx/gl/GLContext.h
> @@ +564,5 @@
> >      /*** In GL debug mode, we completely override glGetError ***/
> >  
> >      GLenum fGetError()
> >      {
> > +        mCachedState.Clear();
> 
> Why is this done?

iirc, any GL calls done since an error occurred are ignored. That would mean our cached values could be completely hosed, so we clear the cache.

> 
> @@ +737,5 @@
> >  // GL official entry points
> >  public:
> >  
> >      void fActiveTexture(GLenum texture) {
> > +        if (mCachedState.mActiveTexture == texture) {
> 
> Couldn't we just set up the API such that the meme was:
> void fFoo(bar) {
>   if (mCachedState.Foo(bar)) {
>     //skipped
>     return
>   }
>   fFoo(bar)
> }
> 
> This way none of the details of mCachedState get leaked into the GLContext
> funcs.

Yes, that's much better. Done that everywhere.

> 
> @@ +2971,5 @@
> > +                target != LOCAL_GL_TEXTURE_RECTANGLE_ARB) {
> > +                return nullptr;
> > +            }
> > +
> > +            static_assert(LOCAL_GL_TEXTURE2 - LOCAL_GL_TEXTURE0 == 2, "Must be sequential!");
> 
> Not really needed. This is Just True for OGL, same as for COLOR_ATTACHMENTi
> and cubemap image enum ranges.

I thought it was nice to document this assertion, but removed.

Comment 49

[Matt Woodrow (:mattwoodrow)]

(In reply to Benoit Jacob [:bjacob] from comment #37)
> If this needs to be an array, then make it a nsTAutoArray so you can get
> memory safety and convenient methods such as Clear().

nsTAutoArray is still a vector, not an array of fixed length (even though it allocates storage to accommodate a fixed length). That would mean trying to access index 2 after calling Clear() would be invalid.


> 
> @@ +3064,5 @@
> > +            void ClearAttribs()
> > +            {
> > +              mSize = 0;
> > +              mStride = 0;
> > +              mPointer = 0;
> 
> Why not clear all?

Could do, clearing those seemed to be enough to guarantee future calls wouldn't match. The other values could reasonably be 0 already, so clearing wouldn't be useful.

> 
> @@ +3068,5 @@
> > +              mPointer = 0;
> > +            }
> > +
> > +            bool mEnabledKnown;
> > +            bool mEnabled;
> 
> EnabledKnown and Enabled sound like they wanted to be a 3-state enum, not a
> pair of two bools (when EnabledKnown is false, Enabled is meaningless,
> right?). This would allow to simplify the above code.

I don't think this is worth it. We have other values that need a 'known' value as well as an integer, so 3-state values aren't possible. I think it's better to just use a bool for all of them.

Comment 50

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache the value of glVertexAttribArray

Comment 51

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

> > ::: gfx/gl/GLContext.h
> > @@ +564,5 @@
> > >      /*** In GL debug mode, we completely override glGetError ***/
> > >  
> > >      GLenum fGetError()
> > >      {
> > > +        mCachedState.Clear();
> > 
> > Why is this done?
> 
> iirc, any GL calls done since an error occurred are ignored. That would mean
> our cached values could be completely hosed, so we clear the cache.

That's not the case. The GL error status has no such effect as ignoring future GL calls.

Comment 52

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

Comment on attachment 8386542 [details] [diff] [review]
Cache the value of glActiveTexture

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

Another round would be needed here; also, see below review comment, there is an issue here that should be caught by running the WebGL conformance tests with a build where this caching is enabled (so in particular is enabled for the GLContext backing a WebGLContext).

::: gfx/gl/GLContext.h
@@ +566,5 @@
>      /*** In GL debug mode, we completely override glGetError ***/
>  
>      GLenum fGetError()
>      {
> +        mCachedState.Clear();

...so (comment 48 and comment 51) this line of code seems to be motivated only by an incorrect assumption, so I suppose that it should be removed.

My biggest concern here is that GetError is called frequently when debugging GL, for example (but not only) in our own GL debug mode, so having GetError clear caches means that we will change a lot of our GL usage when debugging, i.e. we'll be debugging something different.

@@ +742,5 @@
>      void fActiveTexture(GLenum texture) {
> +        if (mCachedState.ActiveTexture(texture)) {
> +            BEFORE_GL_CALL_SKIPPED("%i", texture);
> +            return;
> +        }

So in the initial state here, mActiveTexture is 0. If I call fActiveTexture(0), this should generate a GL_INVALID_ENUM error, but the early-return above means we don't generate any GL error here. This will, in particular, break WebGL conformance.

@@ +2934,5 @@
>  
> +    // GL state cache
> +    struct CachedState {
> +        CachedState() {
> +            Clear();

This has mActiveTexture start with the value 0, but the correct initial value per GL spec is GL_TEXTURE0, which is not the same.

@@ +2942,5 @@
> +            memset(this, 0, sizeof(CachedState));
> +        }
> +
> +        bool ActiveTexture(GLenum texture) {
> +#ifdef MOZ_ENABLE_GL_CACHING

Do we really need a compile-time switch here? Who will want to disable caching at compile time? And if this is not defined, then you'll currently get a 'unused texture parameter' warning here.

Comment 53

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #49)
> (In reply to Benoit Jacob [:bjacob] from comment #37)
> > If this needs to be an array, then make it a nsTAutoArray so you can get
> > memory safety and convenient methods such as Clear().
> 
> nsTAutoArray is still a vector, not an array of fixed length (even though it
> allocates storage to accommodate a fixed length). That would mean trying to
> access index 2 after calling Clear() would be invalid.

You're right, so Clear() wouldn't be what you want here; instead, you would want to fill the nsTAutoArray with zeros.

I still believe that the safety that that would get you, compared to raw arrays, would be worth it!

Comment 54

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

Comment on attachment 8386544 [details] [diff] [review]
Cache the value of glBindTexture

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

R- for the same conformance problem as with the previous patch:

::: gfx/gl/GLContext.h
@@ +2969,5 @@
> +                return false;
> +            }
> +
> +            uint32_t index = mActiveTexture - LOCAL_GL_TEXTURE0;
> +            GLuint *dest;

For extra safety, I would initialize dest to nullptr...

@@ +2974,5 @@
> +            if (target == LOCAL_GL_TEXTURE_2D) {
> +              dest = &mBindTexture2D[0];
> +            } else {
> +              dest = &mBindTextureRect[0];
> +            }

This 'dest' pointer business here could be a lot safer with the nsTAutoArray suggestion, as you could have dest be a nsTAutoArray& reference initialized by a ternary ?: expression.

@@ +2977,5 @@
> +              dest = &mBindTextureRect[0];
> +            }
> +
> +            if (dest[index] == texture) {
> +                return true;

So in the default state here, dest[index] is zero, so if we call glBindTexture with texture=0, we should again get some INVALID_ENUM or INVALID_VALUE error, but instead the return true here means fBindTexture will early-return without generating a GL error, so the same comments as on the previous patch apply --- please try running WebGL conformance tests to validate that caching does not affect GL conformance.

@@ +3011,3 @@
>      private:
>          GLenum mActiveTexture;
> +        GLuint mBindTexture2D[3];

should this be mBoundTexture2D ?

Comment 55

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

Comment on attachment 8386545 [details] [diff] [review]
Cache the value of glUseProgram

I'm going to cancel reviews now because remaining patches seem to all have the same conformance problem (unless I'm wrong).

Also, I'm on vacation for a week, so consider asking :jgilbert for review instead.

Comment 56

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

Comment on attachment 8386544 [details] [diff] [review]
Cache the value of glBindTexture

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

::: gfx/gl/GLContext.h
@@ +2977,5 @@
> +              dest = &mBindTextureRect[0];
> +            }
> +
> +            if (dest[index] == texture) {
> +                return true;

Ouch, sorry, I'm completely wrong here :) mixing up texture units and texture objects. Of course zero is a valid value here. That makes this patch a r+, but please still do consider the other nits.

Comment 57

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

Comment on attachment 8386545 [details] [diff] [review]
Cache the value of glUseProgram

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

actually, this is an easy r+.

Comment 58

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

Comment on attachment 8386546 [details] [diff] [review]
Cache the value of glBindBuffer

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

::: gfx/gl/GLContext.h
@@ +3005,5 @@
>              return false;
>          }
>  
> +        bool BindBuffer(GLenum target, GLuint buffer) {
> +#ifdef MOZ_ENABLE_GL_CACHING

I'm still unsure why we have a MOZ_ENABLE_GL_CACHING compile-time switch, and how this will fare with --enable-warnings-as-errors.

Comment 59

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

Comment on attachment 8386551 [details] [diff] [review]
Cache the value of glVertexAttribArray

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

::: gfx/gl/GLContext.h
@@ +3139,5 @@
> +            GLenum mType;
> +            realGLboolean mNormalized;
> +            GLsizei mStride;
> +            const GLvoid* mPointer;
> +        } mVertexAttribs[CACHED_VERTEX_ATTRIB_COUNT];

I would still like arrays to be nsTAutoArrays for addressing safety.

Comment 60

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ceb23e4d7554
https://hg.mozilla.org/integration/mozilla-inbound/rev/badf9a3d3f79

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ceb23e4d7554
https://hg.mozilla.org/mozilla-central/rev/badf9a3d3f79

Comment 62

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.