843667 - Implement WebGL EXT_draw_buffers

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement)

Description

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

http://www.khronos.org/registry/webgl/extensions/EXT_draw_buffers

Comment 1

[Guillaume Abadie]

Attachment: patch

Implement the WEBGL_draw_buffers usable by MOZ_WEBGL_draw_buffers and fix a bug about the checkFramebufferStatus for shadow mapping.

Comment 2

[Guillaume Abadie]

Attachment: patch revision 1

patch revision 1

Comment 3

[Guillaume Abadie]

Attachment: patch revision 2

patch revision 2 after oral revision with :bjacob

Comment 4

[Guillaume Abadie]

Attachment: patch revision 3

patch revision 3 after offline discussion with bjacob. Introduce the user preference webgl.enable-draft-extensions.

Comment 5

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

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

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

::: content/canvas/src/WebGLContext.cpp
@@ +1107,5 @@
>      else if (CompareWebGLExtensionName(name, "WEBGL_depth_texture"))
>      {
>          ext = WEBGL_depth_texture;
>      }
> +    else if (Preferences::GetBool("webgl.enable-draft-extensions", false))

This is already taken care of in IsExtensionSupported.

@@ +1127,5 @@
>  
>      // step 3: if the extension hadn't been previously been created, create it now, thus enabling it
>      if (!IsExtensionEnabled(ext)) {
>          WebGLExtensionBase *obj = nullptr;
> +        MakeContextCurrent();

Befriend WebGLExtensionDrawBuffers and do it there.

@@ +1255,5 @@
>          gl->GetUIntegerv(LOCAL_GL_STENCIL_BITS, &stencilBits);
>          GLuint stencilMask = (GLuint(1) << stencilBits) - 1;
>  
>          MOZ_ASSERT( ( stencilWriteMaskFront & stencilMask) ==
> +                   (mStencilWriteMaskFront & stencilMask) );

Unwanted whitespace change.

@@ +1267,5 @@
>      // Prepare GL state for clearing.
>      gl->fDisable(LOCAL_GL_SCISSOR_TEST);
>  
>      if (initializeColorBuffer) {
> +        GLenum currentDrawBuffers[WebGLContext::sMaxColorAttachment];

Try to refactor this code to be closed to the old code: do only one clear. Assuming that Clear does honor DrawBuffers, you should be able to, right? (Alternatively, a fallback route might be to try to see if you can use ColorMaski).

@@ +1283,5 @@
> +            if (!colorAttachmentsMask) {
> +                continue;
> +            }
> +
> +            gl->fDrawBuffer(LOCAL_GL_COLOR_ATTACHMENT0 + i);

Careful, this doesn't exist on ES (we'd have a null func ptr there, and likely abort/crash). Use DrawBuffers.

@@ +1284,5 @@
> +                continue;
> +            }
> +
> +            gl->fDrawBuffer(LOCAL_GL_COLOR_ATTACHMENT0 + i);
> +            gl->fClear(LOCAL_GL_COLOR_BUFFER_BIT);

Check that Clear does honor DrawBuffers.

@@ +1548,5 @@
>          arr.AppendElement(NS_LITERAL_STRING("MOZ_WEBGL_depth_texture"));
>      if (IsExtensionSupported(cx, WEBGL_depth_texture))
>          arr.AppendElement(NS_LITERAL_STRING("WEBGL_depth_texture"));
> +
> +    if (Preferences::GetBool("webgl.enable-draft-extensions", false)) {

IsExtensionSupported does that check already.

::: content/canvas/src/WebGLContext.h
@@ +230,5 @@
>      uint32_t Generation() { return mGeneration.value(); }
>  
>      const WebGLRectangleObject *FramebufferRectangleObject() const;
>  
> +    static const size_t sMaxColorAttachment = 16;

Add a 's' to this name.

::: content/canvas/src/WebGLContextGL.cpp
@@ +589,5 @@
> +        }
> +
> +        /* http://www.khronos.org/opengles/sdk/docs/man/xhtml/glCheckFramebufferStatus.xml
> +         GL_FRAMEBUFFER_INCOMPLETE_MISSING_ATTACHMENT
> +         No images are attached to the framebuffer.

Prefer spec over manpages for this kind of comments (more authoritative).

@@ -2252,5 @@
>              return WebGLObjectAsJSValue(cx, mBoundCubeMapTextures[mActiveTexture].get(), rv);
>          }
> -
> -        default:
> -            ErrorInvalidEnumInfo("getParameter: parameter", pname);

Leave a default case, even trivial, for warnings-as-errors.

@@ +2387,5 @@
>      }
>  
>      MakeContextCurrent();
>  
> +    mBoundFramebuffer->colorAttachmentSilentResize(attachment);

ensureColorAttachments

@@ +5525,5 @@
>          framebuffer = framebuffer->getNext())
>      {
> +        for (size_t i = 0; i < size_t(mGLMaxColorAttachments); i++)
> +        {
> +            if (framebuffer->ColorAttachment(i).Texture() == tex) {

|i| should stop at framebuffer's colorattachments count, not the general mGLMaxColorAttachments.

::: content/canvas/src/WebGLExtensionDrawBuffers.cpp
@@ +45,5 @@
> +}
> +
> +void WebGLExtensionDrawBuffers::DrawBuffersWEBGL(const dom::Sequence<GLenum>& buffers)
> +{
> +    size_t buffersLength = buffers.Length();

const?

@@ +64,5 @@
> +         contexts. If DrawBuffersEXT is supplied with a constant other than
> +         BACK and NONE, the error INVALID_OPERATION is generated.
> +         */
> +        if (buffersLength != 1) {
> +            return mContext->ErrorInvalidValue("drawBuffersWEBGL: invalid <buffers> (main BFO: buffers.length must be 1)");

BFO -> framebuffer

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +389,5 @@
>          return false;
>  
>      if (HasIncompleteAttachment())
>          return false;
> +    

trailing whitespace.

@@ +396,5 @@
> +    {
> +        bool checkAllAttachment = true;
> +
> +        for (size_t i = 0; i < colorAttachmentCount; i++) {
> +            checkAllAttachment &= !mColorAttachments[i].HasUninitializedRenderbuffer();

Can we have something simpler than &=! ?

Also, checkAllAttachment is not a good name.

@@ +485,5 @@
> +        if (attachment > LOCAL_GL_COLOR_ATTACHMENT0 &&
> +            attachment <= LOCAL_GL_COLOR_ATTACHMENT15)
> +        {
> +            mContext->ErrorInvalidEnum("%s: attachment: invalid enum value 0x%x, "
> +                                       "you should try WEBGL_draw_buffers extension if supported.", functionName, attachment);

"%s: attachment: invalid enum value 0x%x. "
"Try the WEBGL_draw_buffers extension if supported."

@@ +498,5 @@
> +
> +    return true;
> +}
> +
> +void WebGLFramebuffer::colorAttachmentSilentResize(size_t colorAttachmentId) {

ensureColorAttachments?

::: content/canvas/src/WebGLFramebuffer.h
@@ +159,5 @@
>      WebGLuint mGLName;
>      bool mHasEverBeenBound;
>  
> +    void colorAttachmentSilentResize(size_t colorAttachmentId);
> +    

Trailing \w.

::: dom/bindings/Bindings.conf
@@ +1242,5 @@
>  
> +'WebGLExtensionDrawBuffers': {
> +   'nativeType': 'mozilla::WebGLExtensionDrawBuffers',
> +   'headerFile': 'WebGLExtensions.h'
> +},

Move this to where other extensions are.

Comment 6

[Guillaume Abadie]

Attachment: patch revision 4

Comment 7

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

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=aa2d65138b87

Comment 8

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

Crash in WebGLFramebuffer::CheckAndInitializeRenderbuffers.

Comment 9

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

Specifically in this test:
conformance/misc/object-deletion-behaviour.html

Comment 10

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

debug try: https://tbpl.mozilla.org/?tree=Try&rev=b938cee47fbd

Comment 11

[Guillaume Abadie]

Attachment: path revision 5

Comment 12

[Guillaume Abadie]

Attachment: patch revision 6

Comment 13

[Guillaume Abadie]

Attachment: patch revision 7

Comment 14

[Guillaume Abadie]

Attachment: patch revision 8

Comment 15

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

https://tbpl.mozilla.org/?tree=Try&rev=ee997b97eaf4

Comment 16

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

Comment on attachment 765939 [details] [diff] [review]
patch revision 8

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

Just one more round! See the comment about makecontextcurrent.

::: content/canvas/src/WebGLContext.cpp
@@ +1269,5 @@
>      if (initializeColorBuffer) {
> +
> +        if (drawBuffersIsEnabled) {
> +
> +            GLenum drawBufferCommand[WebGLContext::sMaxColorAttachments] = { LOCAL_GL_NONE };

Rename that to drawBuffersCommand so it's consistent with the function name?

::: content/canvas/src/WebGLExtensionDrawBuffers.cpp
@@ +115,5 @@
> +}
> +
> +bool WebGLExtensionDrawBuffers::IsSupported(const WebGLContext* context)
> +{
> +    gl::GLContext * gl = context->GL();

Oh, you need a context->MakeContextCurrent() here! before the getintegerv calls.

Comment 17

[Guillaume Abadie]

Attachment: patch for landing

Comment 18

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

Comment on attachment 765986 [details] [diff] [review]
patch for landing

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

::: content/canvas/src/WebGLExtensionDrawBuffers.cpp
@@ +69,5 @@
> +        if (buffersLength != 1) {
> +            return mContext->ErrorInvalidValue("drawBuffersWEBGL: invalid <buffers> (main framebuffer: buffers.length must be 1)");
> +        }
> +        else if (buffers[0] == LOCAL_GL_NONE) {
> +            mContext->GL()->fDrawBuffer(LOCAL_GL_NONE);

Actually, you also need a MakeContextCurrent here !

Comment 19

[Guillaume Abadie]

Attachment: patch for final landing

Comment 20

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

Comment on attachment 766017 [details] [diff] [review]
patch for final landing

The tree is still closed; in fact, the only reasonable way to land anything, that we have left, is checkin?, so let's use that. So this likely won't make Firefox 24, sorry.

Comment 21

[(no longer active)]

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

Comment 22

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 766017 [details] [diff] [review]
patch for final landing

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

I saw a few things that looked a little weird, so I went through everything to check. This is mostly nits, but there are some real issues in here. Since there's nothing that would require this to be backed out, this is feedback-, but we should get a follow-up patch together for this. (And probably chat about some of the style nits)

::: content/canvas/src/WebGLContext.cpp
@@ +1173,5 @@
>  
>  void
>  WebGLContext::ClearScreen()
>  {
> +    bool colorAttachmentsMask[WebGLContext::sMaxColorAttachments] = {false};

This is gross stuff. Please please please leave a comment about what this actually does. It would probably be preferable to do this explicitly with std::fill().

@@ +1205,5 @@
>  
>      bool initializeColorBuffer = 0 != (mask & LOCAL_GL_COLOR_BUFFER_BIT);
>      bool initializeDepthBuffer = 0 != (mask & LOCAL_GL_DEPTH_BUFFER_BIT);
>      bool initializeStencilBuffer = 0 != (mask & LOCAL_GL_STENCIL_BUFFER_BIT);
> +    bool drawBuffersIsEnabled = IsExtensionEnabled(WEBGL_draw_buffers);

Why do we pull this out into a bool? This just aliases this info into a form we're less familiar with. The ifs below would probably be clearer if they used this directly.

@@ +1275,5 @@
>      if (initializeColorBuffer) {
> +
> +        if (drawBuffersIsEnabled) {
> +
> +            GLenum drawBuffersCommand[WebGLContext::sMaxColorAttachments] = { LOCAL_GL_NONE };

Please initialize this reasonably as well. Also assert that `mGLMaxDrawBuffers <= WebGLContext::sMaxColorAttachments`.

::: content/canvas/src/WebGLContext.h
@@ +230,5 @@
>      uint32_t Generation() { return mGeneration.value(); }
>  
>      const WebGLRectangleObject *FramebufferRectangleObject() const;
>  
> +    static const size_t sMaxColorAttachments = 16;

Why do we stop at 16? Why bother with size_t for such a small number?

Also, this is a constant, not just a static, so the prefix should be 'k' not 's'. (Using 's' makes things like array length declarations look really suspect.

::: content/canvas/src/WebGLContextGL.cpp
@@ +578,5 @@
> +    hasImages |= mBoundFramebuffer->StencilAttachment().IsDefined();
> +    hasImages |= mBoundFramebuffer->DepthStencilAttachment().IsDefined();
> +
> +    if (!hasImages) {
> +        int32_t colorAttachmentCount = mBoundFramebuffer->mColorAttachments.Length();

Don't use sized types if the size doesn't matter.

::: content/canvas/src/WebGLExtensionDrawBuffers.cpp
@@ +15,5 @@
> +using namespace gl;
> +
> +WebGLExtensionDrawBuffers::WebGLExtensionDrawBuffers(WebGLContext* context)
> +    : WebGLExtensionBase(context)
> +{

We should probably assert that IsSupported is true, unless there's a case where it might not be, but where we're creating the ext object anyways.

@@ +19,5 @@
> +{
> +    GLint maxColorAttachments = 0;
> +    GLint maxDrawBuffers = 0;
> +
> +    gl::GLContext* gl = context->GL();

`gl::` prefix unnecessary since we're `using namespace gl`.

@@ +53,5 @@
> +    if (buffersLength == 0) {
> +        return mContext->ErrorInvalidValue("drawBuffersWEBGL: invalid <buffers> (buffers must not be empty)");
> +    }
> +
> +    if (mContext->mBoundFramebuffer == 0)

Use `== nullptr` or just negate it. Don't mix things up with int(0).

@@ +72,5 @@
> +
> +        mContext->MakeContextCurrent();
> +
> +        if (buffers[0] == LOCAL_GL_NONE) {
> +            const GLenum drawBufffersCommand = LOCAL_GL_NONE;

`drawBufffersCommand` -> `drawBuffersCommand`

::: content/canvas/src/WebGLExtensions.h
@@ +165,5 @@
> +    static const size_t sMinDrawBuffers = 4;
> +    /*
> +     WEBGL_draw_buffers does not give a minal value for GL_MAX_DRAW_BUFFERS. But, we request
> +     for GL_MAX_DRAW_BUFFERS = 4 at least to be able to use all requested color attachements.
> +     See DrawBuffersWEBGL in WebGLExtensionDrawBuffers.cpp inner comments for more informations.

I can't find the comment this mentions. I would also limit these to two, not four. (Limiting to one, while valid, is probably not helpful.)

Really, these number should roughly reflect the worst device we can find. If the worst device we can find which still has this ext has 4 each, then 4 is fine, but we should also note as such explicitly.

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +192,2 @@
>  
> +        size_t colorAttachmentId = size_t(attachment - LOCAL_GL_COLOR_ATTACHMENT0);

Why `size_t` when our type is limited by `GLenum`?

@@ +285,5 @@
>  }
>  
> +bool
> +WebGLFramebuffer::HasIncompleteAttachment() const {
> +    int32_t colorAttachmentCount = mColorAttachments.Length();

Why `int32_t`?

@@ +288,5 @@
> +WebGLFramebuffer::HasIncompleteAttachment() const {
> +    int32_t colorAttachmentCount = mColorAttachments.Length();
> +
> +    for (int32_t i = 0; i < colorAttachmentCount; i++)
> +    {

I like block style too, but it's not moz style, except where we use it for readability for multi-line conditionals.

@@ +302,5 @@
> +}
> +
> +bool
> +WebGLFramebuffer::HasAttachmentsOfMismatchedDimensions() const {
> +    int32_t colorAttachmentCount = mColorAttachments.Length();

Why do we pull this out into its own variable if we only ever use it in one place? Unless we have decent reason to believe this could be a perf concern, we should write mostly for clarity.

@@ +327,5 @@
>      if (attachment == LOCAL_GL_STENCIL_ATTACHMENT)
>          return mStencilAttachment;
>  
> +    if (!CheckColorAttachementNumber(attachment, "getAttachment")) {
> +        NS_ABORT();

I don't like this structure instead of assertions. The usual pattern is to assert what we want to be true and rely on testing to validate this, then just have the non-debug code rely on these assumptions.

If it's something we might hit in non-debug code, we should probably RUNTIME_ASSERT, so we will catch it on nightly, instead of trying to give back *an* answer to try to limp along on. Giving back a random answer on failure is something which might inadvertently be relied upon later on, and cause some truly bizarre bugs.

Basically, we should have two types of code that call this function:
1) Code dependent on user input, for which we should propagate the error information further down, so we can throw or emit a GL error.

2) Internal code not controlled by the user, where we should never ever be calling this wrong, and should assert strongly as such.

@@ +347,5 @@
> +    int32_t colorAttachmentCount = mColorAttachments.Length();
> +
> +    for (int32_t i = 0; i < colorAttachmentCount; i++) {
> +        if (mColorAttachments[i].Texture() == tex) {
> +            FramebufferTexture2D(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_COLOR_ATTACHMENT0, LOCAL_GL_TEXTURE_2D, nullptr, 0);

`LOCAL_GL_COLOR_ATTACHMENT0 + i`

@@ +365,5 @@
>  WebGLFramebuffer::DetachRenderbuffer(const WebGLRenderbuffer *rb) {
> +    int32_t colorAttachmentCount = mColorAttachments.Length();
> +
> +    for (int32_t i = 0; i < colorAttachmentCount; i++) {
> +        if (mColorAttachments[0].Renderbuffer() == rb) {

`i` not `0`

@@ +366,5 @@
> +    int32_t colorAttachmentCount = mColorAttachments.Length();
> +
> +    for (int32_t i = 0; i < colorAttachmentCount; i++) {
> +        if (mColorAttachments[0].Renderbuffer() == rb) {
> +            FramebufferRenderbuffer(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_COLOR_ATTACHMENT0, LOCAL_GL_RENDERBUFFER, nullptr);

`LOCAL_GL_COLOR_ATTACHMENT0 + i`

@@ +392,5 @@
>          return false;
>  
> +    size_t colorAttachmentCount = size_t(mColorAttachments.Length());
> +
> +    {

Why do you introduce an anonymous scope here? Leave a comment when you do this.

@@ +422,5 @@
>      if (status != LOCAL_GL_FRAMEBUFFER_COMPLETE)
>          return false;
>  
>      uint32_t mask = 0;
> +    bool colorAttachmentsMask[WebGLContext::sMaxColorAttachments] = { false };

Please fill this explicitly, or leave a comment that explains what it does.

@@ +468,5 @@
>  
>      return true;
>  }
>  
> +bool WebGLFramebuffer::CheckColorAttachementNumber(WebGLenum attachment, const char * functionName) const

Pick a side for the `*`.

@@ +503,5 @@
> +
> +void WebGLFramebuffer::EnsureColorAttachments(size_t colorAttachmentId) {
> +    size_t currentAttachmentCount = mColorAttachments.Length();
> +
> +    if (mColorAttachments.Length() > colorAttachmentId) {

Leave a comment as to why we're early-outing here. Otherwise, this looks like it's backwards. It's not obvious that this function is ensuring we have sufficient size. It might benefit from a better name.

::: gfx/gl/GLContext.cpp
@@ +88,5 @@
>      "GL_OES_element_index_uint",
>      "GL_OES_vertex_array_object",
>      "GL_ARB_vertex_array_object",
> +    "GL_ARB_draw_buffers",
> +    "GL_EXT_draw_buffers",

Wait, you forgot to actually load the function for these extensions!

`DrawBuffers` is only loaded if we're on desktop GL, and even then, it is considered an optional extension. We should load this function like we load the functions for other extensions, including marking it unsupported if the symbol is missing, and asserting that the pfn is non-null when it's called in GLContext.h.

Also, we need to allow for loading from `DrawBuffersEXT`. (`DrawBuffersARB` as well, if this ext didn't get moved to core by OpenGL 2.0)

Comment 23

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

(In reply to Jeff Gilbert [:jgilbert] from comment #22)
> ::: content/canvas/src/WebGLContext.cpp
> @@ +1173,5 @@
> >  
> >  void
> >  WebGLContext::ClearScreen()
> >  {
> > +    bool colorAttachmentsMask[WebGLContext::sMaxColorAttachments] = {false};
> 
> This is gross stuff. Please please please leave a comment about what this
> actually does. It would probably be preferable to do this explicitly with
> std::fill().

Sorry, what is gross about that? That's the regular way to initialize a static array with a constant value, so do you mean that we shouldn't use static arrays here?

> 
> @@ +1205,5 @@
> >  
> >      bool initializeColorBuffer = 0 != (mask & LOCAL_GL_COLOR_BUFFER_BIT);
> >      bool initializeDepthBuffer = 0 != (mask & LOCAL_GL_DEPTH_BUFFER_BIT);
> >      bool initializeStencilBuffer = 0 != (mask & LOCAL_GL_STENCIL_BUFFER_BIT);
> > +    bool drawBuffersIsEnabled = IsExtensionEnabled(WEBGL_draw_buffers);
> 
> Why do we pull this out into a bool? This just aliases this info into a form
> we're less familiar with. The ifs below would probably be clearer if they
> used this directly.

Agreed, in fact I almost raised this.

> 
> @@ +1275,5 @@
> >      if (initializeColorBuffer) {
> > +
> > +        if (drawBuffersIsEnabled) {
> > +
> > +            GLenum drawBuffersCommand[WebGLContext::sMaxColorAttachments] = { LOCAL_GL_NONE };
> 
> Please initialize this reasonably as well.

Again, I don't understand what's unreasonable here.

> Also assert that
> `mGLMaxDrawBuffers <= WebGLContext::sMaxColorAttachments`.

good idea! (I haven't checked back specifically here --- but it's a good idea to check this anywhere we rely on this condition being true).

> 
> ::: content/canvas/src/WebGLContext.h
> @@ +230,5 @@
> >      uint32_t Generation() { return mGeneration.value(); }
> >  
> >      const WebGLRectangleObject *FramebufferRectangleObject() const;
> >  
> > +    static const size_t sMaxColorAttachments = 16;
> 
> Why do we stop at 16?

Per spec!

> Why bother with size_t for such a small number?

In C/C++, size_t is the standard type to represent anything that's like an array size. Using any /other/ type would require justification. No?

> 
> Also, this is a constant, not just a static, so the prefix should be 'k' not
> 's'. (Using 's' makes things like array length declarations look really
> suspect.

Do we have a coding style document somewhere with such rules? (I like it though, a 'k' does konvey better that it's konstant).

> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +578,5 @@
> > +    hasImages |= mBoundFramebuffer->StencilAttachment().IsDefined();
> > +    hasImages |= mBoundFramebuffer->DepthStencilAttachment().IsDefined();
> > +
> > +    if (!hasImages) {
> > +        int32_t colorAttachmentCount = mBoundFramebuffer->mColorAttachments.Length();
> 
> Don't use sized types if the size doesn't matter.

Indeed, size_t should be the default choice here. But there are many cases where we have to deal with existing mozilla code expecting int32_t or uint32_t. Haven't checked back here.

> 
> ::: content/canvas/src/WebGLExtensionDrawBuffers.cpp
> @@ +15,5 @@
> > +using namespace gl;
> > +
> > +WebGLExtensionDrawBuffers::WebGLExtensionDrawBuffers(WebGLContext* context)
> > +    : WebGLExtensionBase(context)
> > +{
> 
> We should probably assert that IsSupported is true, unless there's a case
> where it might not be, but where we're creating the ext object anyways.

Good idea.

...that's all for tonight, late here ;-)

Comment 24

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

(In reply to Benoit Jacob [:bjacob] from comment #23)
> In C/C++, size_t is the standard type to represent anything that's like an
> array size. Using any /other/ type would require justification. No?

size_t has one obnoxious property -- it's 64-bit on at least the Win64 ABI.  This is only obnoxious when mixing it with 32-bit types instead of using a 32-bit type in the first place.  Don't think it matters here, but just something to keep in mind.

Comment 25

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

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #24)
> (In reply to Benoit Jacob [:bjacob] from comment #23)
> > In C/C++, size_t is the standard type to represent anything that's like an
> > array size. Using any /other/ type would require justification. No?
> 
> size_t has one obnoxious property -- it's 64-bit on at least the Win64 ABI. 

size_t is 64-bit on all architectures with more-than-32bit-segments, by definition, being the type of malloc's size argument. In particular it's unconditionally 64-bit on x86-64 --- regardless of the choice of ABI.

> This is only obnoxious when mixing it with 32-bit types instead of using a
> 32-bit type in the first place.  Don't think it matters here, but just
> something to keep in mind.

Indeed. That's why we don't always use size_t. I was just saying it's the /default/ choice for a size-like thing, you're right that there are going to be exceptions and that would indeed be a typical reason for exceptions.

Comment 26

[Phil Ringnalda (:philor)]

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

Comment 27

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #23)
> (In reply to Jeff Gilbert [:jgilbert] from comment #22)
> > ::: content/canvas/src/WebGLContext.cpp
> > @@ +1173,5 @@
> > >  
> > >  void
> > >  WebGLContext::ClearScreen()
> > >  {
> > > +    bool colorAttachmentsMask[WebGLContext::sMaxColorAttachments] = {false};
> > 
> > This is gross stuff. Please please please leave a comment about what this
> > actually does. It would probably be preferable to do this explicitly with
> > std::fill().
> 
> Sorry, what is gross about that? That's the regular way to initialize a
> static array with a constant value, so do you mean that we shouldn't use
> static arrays here?
It's gross because what this looks like it initializes this to a one-element array. (Not true, but what it looks like) The next guess would be that it fills the array with `false`. It doesn't technically do this, though in practice, this is the result. It really is setting the first element to `false`, and either 0-filling the rest, or casting zeros into the remaining elements. Notably, changing this to `{true}` means the array is now `{true, false, false,...}`. This requires much more thinking than I should need to do for code like this.
> 
> > 
> > @@ +1275,5 @@
> > >      if (initializeColorBuffer) {
> > > +
> > > +        if (drawBuffersIsEnabled) {
> > > +
> > > +            GLenum drawBuffersCommand[WebGLContext::sMaxColorAttachments] = { LOCAL_GL_NONE };
> > 
> > Please initialize this reasonably as well.
> 
> Again, I don't understand what's unreasonable here.
This is even more treacherous, though LOCAL_GL_NONE is indeed 0, so it shouldn't matter. (here, at least)
> 
> > Also assert that
> > `mGLMaxDrawBuffers <= WebGLContext::sMaxColorAttachments`.
> 
> good idea! (I haven't checked back specifically here --- but it's a good
> idea to check this anywhere we rely on this condition being true).
> 
> > 
> > ::: content/canvas/src/WebGLContext.h
> > @@ +230,5 @@
> > >      uint32_t Generation() { return mGeneration.value(); }
> > >  
> > >      const WebGLRectangleObject *FramebufferRectangleObject() const;
> > >  
> > > +    static const size_t sMaxColorAttachments = 16;
> > 
> > Why do we stop at 16?
> 
> Per spec!
Cool, needs a comment! :)
> 
> > Why bother with size_t for such a small number?
> 
> In C/C++, size_t is the standard type to represent anything that's like an
> array size. Using any /other/ type would require justification. No?
For variables which are always going to be small, usually readability is nicer with just `int`. `int` rarely needs justification. `size_t` is technically correct, but definitely overkill here, in my opinion.
> 
> > 
> > Also, this is a constant, not just a static, so the prefix should be 'k' not
> > 's'. (Using 's' makes things like array length declarations look really
> > suspect.
> 
> Do we have a coding style document somewhere with such rules? (I like it
> though, a 'k' does konvey better that it's konstant).
I don't know if we have it written down anywhere. To make constants stand out, they should either be ALL_CAPS like defines or have a k prefix, as stolen from Hungarian Notation, iirc.

Comment 28

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

(In reply to Jeff Gilbert [:jgilbert] from comment #27)
> (In reply to Benoit Jacob [:bjacob] from comment #23)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #22)
> > > ::: content/canvas/src/WebGLContext.cpp
> > > @@ +1173,5 @@
> > > >  
> > > >  void
> > > >  WebGLContext::ClearScreen()
> > > >  {
> > > > +    bool colorAttachmentsMask[WebGLContext::sMaxColorAttachments] = {false};
> > > 
> > > This is gross stuff. Please please please leave a comment about what this
> > > actually does. It would probably be preferable to do this explicitly with
> > > std::fill().
> > 
> > Sorry, what is gross about that? That's the regular way to initialize a
> > static array with a constant value, so do you mean that we shouldn't use
> > static arrays here?
> It's gross because what this looks like it initializes this to a one-element
> array. (Not true, but what it looks like) The next guess would be that it
> fills the array with `false`. It doesn't technically do this, though in
> practice, this is the result. It really is setting the first element to
> `false`, and either 0-filling the rest, or casting zeros into the remaining
> elements. Notably, changing this to `{true}` means the array is now `{true,
> false, false,...}`. This requires much more thinking than I should need to
> do for code like this.

Hm, OK. I would r+ a patch making this occurrence use something with nicer syntax, but I wouldn't take time to do it myself or ask someone else to --- this is really a complaint about a quirk of C/C++ syntax that we are already rely on in various other places.


> > 
> > > 
> > > @@ +1275,5 @@
> > > >      if (initializeColorBuffer) {
> > > > +
> > > > +        if (drawBuffersIsEnabled) {
> > > > +
> > > > +            GLenum drawBuffersCommand[WebGLContext::sMaxColorAttachments] = { LOCAL_GL_NONE };
> > > 
> > > Please initialize this reasonably as well.
> > 
> > Again, I don't understand what's unreasonable here.
> This is even more treacherous, though LOCAL_GL_NONE is indeed 0, so it
> shouldn't matter. (here, at least)
> > 
> > > Also assert that
> > > `mGLMaxDrawBuffers <= WebGLContext::sMaxColorAttachments`.
> > 
> > good idea! (I haven't checked back specifically here --- but it's a good
> > idea to check this anywhere we rely on this condition being true).
> > 
> > > 
> > > ::: content/canvas/src/WebGLContext.h
> > > @@ +230,5 @@
> > > >      uint32_t Generation() { return mGeneration.value(); }
> > > >  
> > > >      const WebGLRectangleObject *FramebufferRectangleObject() const;
> > > >  
> > > > +    static const size_t sMaxColorAttachments = 16;
> > > 
> > > Why do we stop at 16?
> > 
> > Per spec!
> Cool, needs a comment! :)
> > 
> > > Why bother with size_t for such a small number?
> > 
> > In C/C++, size_t is the standard type to represent anything that's like an
> > array size. Using any /other/ type would require justification. No?
> For variables which are always going to be small, usually readability is
> nicer with just `int`. `int` rarely needs justification. `size_t` is
> technically correct, but definitely overkill here, in my opinion.

I disagree: size_t means that it's a size/index-like thing, while int conveys no information besides "we don't care as long as this has at least 32 bits". So while both work, size_t gives some useful self-documentation to this code, while int doesn't.

Comment 29

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #28)
> (In reply to Jeff Gilbert [:jgilbert] from comment #27)
> > (In reply to Benoit Jacob [:bjacob] from comment #23)
> > > (In reply to Jeff Gilbert [:jgilbert] from comment #22)
> > > > ::: content/canvas/src/WebGLContext.cpp
> > > > @@ +1173,5 @@
> > > > >  
> > > > >  void
> > > > >  WebGLContext::ClearScreen()
> > > > >  {
> > > > > +    bool colorAttachmentsMask[WebGLContext::sMaxColorAttachments] = {false};
> > > > 
> > > > This is gross stuff. Please please please leave a comment about what this
> > > > actually does. It would probably be preferable to do this explicitly with
> > > > std::fill().
> > > 
> > > Sorry, what is gross about that? That's the regular way to initialize a
> > > static array with a constant value, so do you mean that we shouldn't use
> > > static arrays here?
> > It's gross because what this looks like it initializes this to a one-element
> > array. (Not true, but what it looks like) The next guess would be that it
> > fills the array with `false`. It doesn't technically do this, though in
> > practice, this is the result. It really is setting the first element to
> > `false`, and either 0-filling the rest, or casting zeros into the remaining
> > elements. Notably, changing this to `{true}` means the array is now `{true,
> > false, false,...}`. This requires much more thinking than I should need to
> > do for code like this.
> 
> Hm, OK. I would r+ a patch making this occurrence use something with nicer
> syntax, but I wouldn't take time to do it myself or ask someone else to ---
> this is really a complaint about a quirk of C/C++ syntax that we are already
> rely on in various other places.
I'll do this then. This is particularly ugly, even for C++. Part of not falling into C++ quirk hell is restraining ourselves not to just what works, but to what's clear. Relying on quirks (and people's knowledge of such quirks) is hardly a good idea. If we do want to use something quirky, we should be commenting about why we've chosen that path instead of the clear and obvious path.
> 
> 
> > > 
> > > > 
> > > > @@ +1275,5 @@
> > > > >      if (initializeColorBuffer) {
> > > > > +
> > > > > +        if (drawBuffersIsEnabled) {
> > > > > +
> > > > > +            GLenum drawBuffersCommand[WebGLContext::sMaxColorAttachments] = { LOCAL_GL_NONE };
> > > > 
> > > > Please initialize this reasonably as well.
> > > 
> > > Again, I don't understand what's unreasonable here.
> > This is even more treacherous, though LOCAL_GL_NONE is indeed 0, so it
> > shouldn't matter. (here, at least)
> > > 
> > > > Also assert that
> > > > `mGLMaxDrawBuffers <= WebGLContext::sMaxColorAttachments`.
> > > 
> > > good idea! (I haven't checked back specifically here --- but it's a good
> > > idea to check this anywhere we rely on this condition being true).
> > > 
> > > > 
> > > > ::: content/canvas/src/WebGLContext.h
> > > > @@ +230,5 @@
> > > > >      uint32_t Generation() { return mGeneration.value(); }
> > > > >  
> > > > >      const WebGLRectangleObject *FramebufferRectangleObject() const;
> > > > >  
> > > > > +    static const size_t sMaxColorAttachments = 16;
> > > > 
> > > > Why do we stop at 16?
> > > 
> > > Per spec!
> > Cool, needs a comment! :)
> > > 
> > > > Why bother with size_t for such a small number?
> > > 
> > > In C/C++, size_t is the standard type to represent anything that's like an
> > > array size. Using any /other/ type would require justification. No?
> > For variables which are always going to be small, usually readability is
> > nicer with just `int`. `int` rarely needs justification. `size_t` is
> > technically correct, but definitely overkill here, in my opinion.
> 
> I disagree: size_t means that it's a size/index-like thing, while int
> conveys no information besides "we don't care as long as this has at least
> 32 bits". So while both work, size_t gives some useful self-documentation to
> this code, while int doesn't.
No, for 'int with >=32 bits', we should be using sized types. `int` is just the standard integer type. If we ever work with arrays that can be large enough to care about size, I completely agree about using size_t. For things like this, that max out at something trivially low as 16, we only really care that it's an integer.

The most annoying part of size_t is how it tends to pollute things with casts to size_t, as seen in this patch. This makes code less immediately readable at negligible benefit to safety.

I don't agree with Google's style guides on everything, but I really like their stance on integer types. Unfortunately, our style guide doesn't mention them.

FWIW, our style guide *does* mention using 'k' as a prefix for constants, though it also has the 'a' prefix for arguments, which we've dropped as needlessly crufty.

Comment 30

[Kelsey Gilbert [:jgilbert]]

Anyways, we don't need to bikeshed here on whether or not to use size_t for all size-related ints, though it should be bikeshedded about somewhere at some point. (GFX style guide, perhaps?)

Comment 31

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

(In reply to Jeff Gilbert [:jgilbert] from comment #29)
> > Hm, OK. I would r+ a patch making this occurrence use something with nicer
> > syntax, but I wouldn't take time to do it myself or ask someone else to ---
> > this is really a complaint about a quirk of C/C++ syntax that we are already
> > rely on in various other places.
> I'll do this then. This is particularly ugly, even for C++. Part of not
> falling into C++ quirk hell is restraining ourselves not to just what works,
> but to what's clear. Relying on quirks (and people's knowledge of such
> quirks) is hardly a good idea. If we do want to use something quirky, we
> should be commenting about why we've chosen that path instead of the clear
> and obvious path.
I guess this just fell into my mental category for C++ syntax quirks that are ugly when you first meet them, but that you then don't think much about. Likewise, I've been used to assuming that GL_NONE == GL_NO_ERROR == 0, and we've been relying on that in so many places, for so long, that it doesn't bother me anymore to rely further on this. But, I have nothing to say against a patch that makes it nicer.

> > I disagree: size_t means that it's a size/index-like thing, while int
> > conveys no information besides "we don't care as long as this has at least
> > 32 bits". So while both work, size_t gives some useful self-documentation to
> > this code, while int doesn't.
> No, for 'int with >=32 bits', we should be using sized types. `int` is just
> the standard integer type.

Note that "int is the standard integer type" is only relevant in few places. That phrase has a few different meanings, e.g. "int is the result of bitwise ops on tiny integer types", that indeed give a special role to int for some use cases... but I don't see any being relevant here. By contrast, size_t being the standard type to represent something that's like a size, seems very relevant here.

> If we ever work with arrays that can be large
> enough to care about size, I completely agree about using size_t. For things
> like this, that max out at something trivially low as 16, we only really
> care that it's an integer.

Even without needing the range of size_t --- the whole language is built around size_t for anything that looks like a size. For example, sizeof returns a size_t, not an int. We could very well want to mix small size constants, such as the one in question here, with some sizeof's. Other examples include: the parameter to new[], the parameter to alignas, the result of alignof... looking beyond the core language, examples abound in the standard library, with e.g. vector::size returning a size_t, and in the C library, strlen returning a size_t, memcpy taking a size_t, etc.

> The most annoying part of size_t is how it tends to pollute things with
> casts to size_t, as seen in this patch. This makes code less immediately
> readable at negligible benefit to safety.

But since the core language and the standard library already want to use size_t for anything that's like a size, you will have to cast to/from size_t anyways. The only way out is to embrace size_t. I see this as something that Gecko needs to gradually be fixed for in the long term, a bit like const-correctness.

That also has performance implications. On 64bit platforms (where int is typically still 32bit, and size_t is by definition 64bit, at least on a platform with 64bit segments), using int's to address arrays can force the compiler to emit extra instructions to convert 32bit integers to 64bit (e.g. clearing the upper 32bits of a register) in order to use it as an offset in 64bit pointer arithmetic.

Don't get me wrong, there are solid reasons to keep [u]int32 sizes in some places. For example, nsTArray::Header represents the length and capacity in 32bit each. The rationale for that is that minimizing sizeof matters there. But that is the non-default case, that needs justification (it does have justification there). The problem is when this propagates to other parts of the codebase without justification (e.g. there is no justification for nsTArray::operator[] taking a uint32 instead of a size_t).

> 
> I don't agree with Google's style guides on everything, but I really like
> their stance on integer types.

So do I. And it doesn't oppose using size_t or recommend int over size_t. It says "Of the built-in C++ integer types, the only one used is int" but obviously it doesn't size_t as a built-in type. Ater all it is std::size_t, much like int64_t is std::int64_t. size_t is a built-in typedef but not a type of its own, I guess. If you unfold the explanation there, it goes on to say:  "When appropriate, you are welcome to use standard types like size_t and ptrdiff_t."  It also says: "We use int very often, for integers we know are not going to be too big, e.g., loop counters." --- note how it only mentions loop counters and not e.g. array addressing! Also note how it says the same as I was saying above about int meaning basically a type that's at least 32bit: "You should assume that an int is at least 32 bits, but don't assume that it has more than 32 bits."

Comment 32

[Guillaume Abadie]

Attachment: patch update 1

this patch fix issues found by Jeff Gilbert in the landed one.

Comment 33

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

Comment on attachment 769053 [details] [diff] [review]
patch update 1

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

::: content/canvas/src/WebGLContext.cpp
@@ +1286,5 @@
>      if (initializeColorBuffer) {
>  
>          if (drawBuffersIsEnabled) {
>  
> +            MOZ_ASSERT(size_t(mGLMaxDrawBuffers) <= WebGLContext::kMaxColorAttachments);

Since this is a compile-time condition, please use a MOZ_STATIC_ASSERT here, so we can get compile-time errors instead of runtime errors.

::: content/canvas/src/WebGLExtensionDrawBuffers.cpp
@@ +55,5 @@
>      if (buffersLength == 0) {
>          return mContext->ErrorInvalidValue("drawBuffersWEBGL: invalid <buffers> (buffers must not be empty)");
>      }
>  
> +    if (mContext->mBoundFramebuffer == nullptr)

I would do: !mContext->mBoundFramebuffer. This is what we do elsewhere, let's be consistent.

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +328,5 @@
>          return mDepthAttachment;
>      if (attachment == LOCAL_GL_STENCIL_ATTACHMENT)
>          return mStencilAttachment;
>  
> +    MOZ_ASSERT(CheckColorAttachementNumber(attachment, "getAttachment"));

Wait! Are you certain that a script can't hit this assertion by passing a bad attachment parameter to this function? We don't want to allow scripts to crash DEBUG builds of Firefox.

::: gfx/gl/GLContext.cpp
@@ +598,5 @@
>                  mSymbols.fDeleteVertexArrays = nullptr;
>              }
>          }
>  
> +        if (mIsGLES2 && IsExtensionSupported(EXT_draw_buffers)) {

I think that you also only don't want to do this if fDrawBuffers is still null. Indeed, if you already have a core or ARB fDrawBuffers, it's probably not useful to look at the EXT one, right?

Comment 34

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

(In reply to Benoit Jacob [:bjacob] from comment #33)
> Comment on attachment 769053 [details] [diff] [review]
> patch update 1
> 
> Review of attachment 769053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +1286,5 @@
> >      if (initializeColorBuffer) {
> >  
> >          if (drawBuffersIsEnabled) {
> >  
> > +            MOZ_ASSERT(size_t(mGLMaxDrawBuffers) <= WebGLContext::kMaxColorAttachments);
> 
> Since this is a compile-time condition, please use a MOZ_STATIC_ASSERT here,
> so we can get compile-time errors instead of runtime errors.

Oh, ignore this comment. I can't read. Sorry.

Comment 35

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

(In reply to Benoit Jacob [:bjacob] from comment #33)
> ::: gfx/gl/GLContext.cpp
> @@ +598,5 @@
> >                  mSymbols.fDeleteVertexArrays = nullptr;
> >              }
> >          }
> >  
> > +        if (mIsGLES2 && IsExtensionSupported(EXT_draw_buffers)) {
> 
> I think that you also only don't want to do this if fDrawBuffers is still
> null. Indeed, if you already have a core or ARB fDrawBuffers, it's probably
> not useful to look at the EXT one, right?

Chatted offline: I was missing that the above path loading core/ARB symbols was specifically non-ES2.

Comment 36

[Guillaume Abadie]

Attachment: patch update 2

Comment 37

[Guillaume Abadie]

has discussed offline, wait for Push to Try before requesting authorization to push on aurora

https://tbpl.mozilla.org/?tree=Try&rev=ace70fb53380

Comment 38

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/43e688b70d84

Comment 39

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

Comment on attachment 769084 [details] [diff] [review]
patch update 2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): WEBGL_draw_buffers, implemented in bug 843667
User impact if declined: crash on some platforms when viewing certain WebGL pages in non-default config (webgl.enable-draft-extensions=true)
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): low risk, though nonzero (dynamic symbol loading).
String or IDL/UUID changes made by this patch: none

Comment 40

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

Compile errors in this log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=24847916&tree=Mozilla-Inbound&full=1#error0

Comment 41

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 769084 [details] [diff] [review]
patch update 2

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

Missed a spot. :)

::: gfx/gl/GLContext.cpp
@@ +599,5 @@
>              }
>          }
>  
> +        if (mIsGLES2 && IsExtensionSupported(EXT_draw_buffers)) {
> +            SymLoadStruct vaoSymbols[] = {

`vaoSymbols`?

Comment 42

[Guillaume Abadie]

Attachment: patch update 3

Ok I reapplied my patch on a last mozilla-central and the problem come from a collision with someone-else patch, at exactly the same line as the build-time was. Therefor this modification have not be applied, causing a built-time error because I have renamed WebGLContext::sMaxColorAttachments to WebGLContext::kMaxColorAttachments

My honor is safe. =)

Therefor in this patch, I have just changed vaoSymbols to mrtSymbols and offcourse, fix the collision problem. And it compile.