1219890 - Support WebGL2 on top of ES3

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

This is needed for ANGLE

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make this work while breaking ES2

Comment 2

[Jeff Muizelaar [:jrmuizel]]

jgilbert, do you have any preferences for how to implement this? Should we duplicate the static arrays with es3 versions or should we build the arrays dynamically on the stack? How should we communicate the version required to the context?

Comment 3

[Kelsey Gilbert [:jgilbert]]

Let's construct arrays. Having static arrays is nice when the options are limited, but not really necessary when we'd have to have a large number.

Also it'd be nice to support printing out our chosen array, perhaps when MOZ_GL_SPEW=1?

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Construct context arguments on the stack

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make assignment operator and copy constructor default.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use es3 when appropriate

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8693074 [details] [diff] [review]
Construct context arguments on the stack

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ +498,5 @@
>  
>      EGLContext eglShareContext = shareContext ? shareContext->mContext
>                                                : EGL_NO_CONTEXT;
> +
> +    nsAutoTArray<EGLint, 6> contextAttribs;

It doesn't hurt to have the `, 6` here, but it's not something we should be worrying about in such cold code. I would omit this as premature optimization.

@@ +499,5 @@
>      EGLContext eglShareContext = shareContext ? shareContext->mContext
>                                                : EGL_NO_CONTEXT;
> +
> +    nsAutoTArray<EGLint, 6> contextAttribs;
> +    contextAttribs.AppendElement(LOCAL_EGL_CONTEXT_CLIENT_VERSION); contextAttribs.AppendElement(2);

Don't put two lines on one line. Surround with newlines to establish that these are a pair. Use a lambda if you want to make it very explicit.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8693075 [details] [diff] [review]
Make assignment operator and copy constructor default.

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

::: gfx/gl/SurfaceTypes.cpp
@@ +15,5 @@
>      Clear();
>  }
>  
> +/* These are defined out of line so that we don't need to include
> + * ISurfaceAllocator.h in the header */

Use `//` for short comments like this.

@@ +21,2 @@
>  SurfaceCaps&
> +SurfaceCaps::operator=(const SurfaceCaps& other) = default;

Tools I wish I had when I wrote this!

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8693082 [details] [diff] [review]
Use es3 when appropriate

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ +500,5 @@
>                                                : EGL_NO_CONTEXT;
>  
>      nsAutoTArray<EGLint, 6> contextAttribs;
> +    contextAttribs.AppendElement(LOCAL_EGL_CONTEXT_CLIENT_VERSION);
> +    if (caps.es3)

Use CreateContextFlags for this.

::: gfx/gl/SurfaceTypes.h
@@ +25,5 @@
>      bool depth, stencil;
>      bool antialias;
>      bool premultAlpha;
>      bool preserve;
> +    bool es3;

This is not how we should pass this down.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use es3 when appropriate v2

How about this one.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/232826e2ee72
https://hg.mozilla.org/integration/mozilla-inbound/rev/811dce914c46
https://hg.mozilla.org/integration/mozilla-inbound/rev/5426f8048840

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/232826e2ee72
https://hg.mozilla.org/mozilla-central/rev/811dce914c46
https://hg.mozilla.org/mozilla-central/rev/5426f8048840