968519 - Try to get a GLES3 context if available on EGL

Status: RESOLVED INCOMPLETE

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Since GLES3 is backwards compatible with GLES2, we should try to get and ES3 context if we can.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Mmm I like it in theory, I wonder if we're likely to hit odd bugs because ES3 is newer?  Then again, we've hit odd bugs in ES2, so maybe "newer" is actually better :)  Stick it behind a pref so we can easily turn it off if needed?

Comment 2

[Kelsey Gilbert [:jgilbert]]

I'm thinking newer is going to be better, but we should have a pref, and this should be easily disablable for if we do have trouble.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 0: Use a scoped helper class to do attrib list building

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: Make out-vars really obvious

Not strictly required, but I think it's an improvement. What do you think?

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Try to get ES3 contexts where possible, before falling back to ES2.

Comment 6

[Kelsey Gilbert [:jgilbert]]

You can totally reassign these if you find someone else to handle them.

Comment 7

[Kelsey Gilbert [:jgilbert]]

I tested this on my Nexus 4 (android 4.4). It gets GLES3 with this patch queue. I think it got ES3 without it, too. (It would depend on the order of the configs, since we only ask for GLES2+ for offscreen contexts. We always ask for GLES2 renderables, though renderables can support both GLES2 and GLES3 bits, I think)

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> We always
> ask for GLES2 renderables, though renderables can support both GLES2 and
> GLES3 bits, I think)

So we don't need to change the junk in GLController.java?

Comment 9

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #8)
> (In reply to Jeff Gilbert [:jgilbert] from comment #7)
> > We always
> > ask for GLES2 renderables, though renderables can support both GLES2 and
> > GLES3 bits, I think)
> 
> So we don't need to change the junk in GLController.java?

I don't think so, but it's possible. It's also possible that we'd need to load a different SO on some machines.

Comment 10

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

Comment on attachment 8371248 [details] [diff] [review]
patch 0: Use a scoped helper class to do attrib list building

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ +190,5 @@
> +class EGLConfigAttribs
> +{
> +private:
> +    std::vector<EGLint> mAttribVector;
> +    ScopedDeleteArray<EGLint> mAttribArr;

I don't want us to start using plain arrays and manual std::copy for what could be done more simply and safely with a std::vector or a nsTArray.

@@ +209,5 @@
> +            Push(LOCAL_EGL_NONE, 0);
> +            Push(0, 0);
> +
> +            // Copy to our array
> +            mAttribArr = new EGLint[mAttribVector.size()];

For example, nothing here would save us if for whatever reason we passed a wrong value here for this plain array's size. With std::vector or nsTArray we get nice assertions.

Note that for once, xpcom containers have an edge over std ones: with nsAutoTArray you can have most cases avoid dynamic memory allocation while automatically switching to it where needed.

Comment 11

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

Comment on attachment 8371249 [details] [diff] [review]
patch 1: Make out-vars really obvious

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

I would suggest naming them aOutConfig to keep existing style. I have no idea whether the coding style says something about that.

Comment 12

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

Comment on attachment 8371250 [details] [diff] [review]
patch 2: Try to get ES3 contexts where possible, before falling back to ES2.

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

Why do we prefer a ES3 context for stuff that can be done as well with a ES2 context?

Have you measured if this makes a difference in terms of memory usage? (DMD would be a good tool to measure that; about:memory rss would already be a start).

Also, what about startup times?

I'd be happy with some crude measurements even on just one Android device.

I think that this review should be deferred until then.

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Benoit Jacob [:bjacob] from comment #12)
> Comment on attachment 8371250 [details] [diff] [review]
> patch 2: Try to get ES3 contexts where possible, before falling back to ES2.
> 
> Review of attachment 8371250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why do we prefer a ES3 context for stuff that can be done as well with a ES2
> context?
> 
> Have you measured if this makes a difference in terms of memory usage? (DMD
> would be a good tool to measure that; about:memory rss would already be a
> start).
> 
> Also, what about startup times?
> 
> I'd be happy with some crude measurements even on just one Android device.
> 
> I think that this review should be deferred until then.

We need ES3 to do FSAA in Skia, but you make good points.

Comment 14

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #13)
> (In reply to Benoit Jacob [:bjacob] from comment #12)
> > Comment on attachment 8371250 [details] [diff] [review]
> > patch 2: Try to get ES3 contexts where possible, before falling back to ES2.
> > 
> > Review of attachment 8371250 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Why do we prefer a ES3 context for stuff that can be done as well with a ES2
> > context?
> > 
> > Have you measured if this makes a difference in terms of memory usage? (DMD
> > would be a good tool to measure that; about:memory rss would already be a
> > start).
> > 
> > Also, what about startup times?
> > 
> > I'd be happy with some crude measurements even on just one Android device.
> > 
> > I think that this review should be deferred until then.
> 
> We need ES3 to do FSAA in Skia, but you make good points.

You mean MSAA. FSAA is just increasing the height and width of the drawbuffer.

Comment 15

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

Even so, why would you need ES3 for that?

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #13)
> > (In reply to Benoit Jacob [:bjacob] from comment #12)
> > > Comment on attachment 8371250 [details] [diff] [review]
> > > patch 2: Try to get ES3 contexts where possible, before falling back to ES2.
> > > 
> > > Review of attachment 8371250 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Why do we prefer a ES3 context for stuff that can be done as well with a ES2
> > > context?
> > > 
> > > Have you measured if this makes a difference in terms of memory usage? (DMD
> > > would be a good tool to measure that; about:memory rss would already be a
> > > start).
> > > 
> > > Also, what about startup times?
> > > 
> > > I'd be happy with some crude measurements even on just one Android device.
> > > 
> > > I think that this review should be deferred until then.
> > 
> > We need ES3 to do FSAA in Skia, but you make good points.
> 
> You mean MSAA. FSAA is just increasing the height and width of the
> drawbuffer.

Oops, sorry, yes.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Benoit Jacob [:bjacob] from comment #15)
> Even so, why would you need ES3 for that?

The relevant extensions (EXT_framebuffer_multisample) don't appear to be present on most devices. ES3 has it in core.

Comment 18

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 0: Use a scoped helper class to do attrib list building

Now with 100% more nsTArray!

Comment 19

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: Make out-vars really obvious

r=bjacob

Comment 20

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: Make out-vars really obvious

You r+'d most of this, but I also removed the forward decl now, since it's not needed.

Comment 21

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

(In reply to James Willcox (:snorp) ([e-mail]) from comment #17)
> (In reply to Benoit Jacob [:bjacob] from comment #15)
> > Even so, why would you need ES3 for that?
> 
> The relevant extensions (EXT_framebuffer_multisample) don't appear to be
> present on most devices. ES3 has it in core.

But surely the devices that support ES3 also support the EXT_framebuffer_multisample extension in ES2 contexts?

Comment 22

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

Comment on attachment 8371845 [details] [diff] [review]
patch 1: Make out-vars really obvious

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ +557,5 @@
>      return ResizeScreenBuffer(aNewSize);
>  }
>  
>  static bool
> +CreateConfig(int32_t depth, EGLConfig* aOut_Config)

Ouch, I haven't seen this kind of Camel_Case anywhere else in our codebase, except in very specific places where we needed to single out a suffix. I don't like that, but I don't care too much.

Comment 23

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

Comment on attachment 8371836 [details] [diff] [review]
patch 0: Use a scoped helper class to do attrib list building

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ +188,5 @@
> +class EGLConfigAttribs
> +{
> +private:
> +    bool mIsFrozen;
> +    nsTArray<EGLint> mAttribs;

If now you replace nsTArray<EGLint> by nsAutoTArray<EGLInt, N> for a suitable value of N, you get to avoid malloc in most cases, without having to worry about anything (it will switch to a heap array if you exceed N ints).

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Why are we doing this at all, especially right now?  The code writing and review energy here could be better spent.

Comment 25

[Kelsey Gilbert [:jgilbert]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #24)
> Why are we doing this at all, especially right now?  The code writing and
> review energy here could be better spent.

Skia needs ES3 to get better coverage with MSAA support, which gives better perf.
To do this, we need to sometimes submit a EGLint[] with ES2 and sometimes with ES3. This would have doubled the number of static EGLint[]s we have, and made things increasingly unwieldy. Instead, I think we should switch to being able to construct these dynamically.

Comment 26

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #22)
> Comment on attachment 8371845 [details] [diff] [review]
> patch 1: Make out-vars really obvious
> 
> Review of attachment 8371845 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContextProviderEGL.cpp
> @@ +557,5 @@
> >      return ResizeScreenBuffer(aNewSize);
> >  }
> >  
> >  static bool
> > +CreateConfig(int32_t depth, EGLConfig* aOut_Config)
> 
> Ouch, I haven't seen this kind of Camel_Case anywhere else in our codebase,
> except in very specific places where we needed to single out a suffix. I
> don't like that, but I don't care too much.

So I'm balancing a few needs here:
* We're (lightly) supposed to use aFoo for args, largely to prevent local variables shadowing args.
* We should designate output variables, to differentiate them from input vars of similar type.
* I want to use and underscore to separate the 'out' from the 'foo', otherwise this looks like 'OutConfig' like 'Config of/about/for-the-purpose-of (going/being?) out', as opposed to 'config, which is an output variable'. This would be needed for consistency if we had something like an outvar 'earlyConfig', for which 'aOut_EarlyConfig' is better (more clear) than 'aOutEarlyConfig'.

I would personally prefer 'out_fooBar'.

Comment 27

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #21)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #17)
> > (In reply to Benoit Jacob [:bjacob] from comment #15)
> > > Even so, why would you need ES3 for that?
> > 
> > The relevant extensions (EXT_framebuffer_multisample) don't appear to be
> > present on most devices. ES3 has it in core.
> 
> But surely the devices that support ES3 also support the
> EXT_framebuffer_multisample extension in ES2 contexts?

snorp was seeing this not be the case, IIRC.

Comment 28

[Kelsey Gilbert [:jgilbert]]

I can't seem to get DMD working with fennec. Can you either help me with this or find me someone who can?

Comment 29

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

You could ask Nick Nethercote, the primary author of DMD.

Comment 30

[Kelsey Gilbert [:jgilbert]]

We may be doing this for ANGLE, and we already accidentally sometimes get ES3 on Android when we request ES2.

Let's shutter this bug and handle this as needed.