903481 - WebGL2 transform feedback - Buffers

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Guillaume Abadie]

Implement WebGL 2 transform feedback buffers

Comment 1

[Guillaume Abadie]

Attachment: patch revision 1

This patch move buffer function into WebGLContextBuffers.cpp, and add support for TRANSFORM_FEEDBACK_BUFFER.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 788398 [details] [diff] [review]
patch revision 1

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

If we don't address things like `index` bounds checking now, we're likely to forget in the future.

::: content/canvas/src/WebGLContextBuffers.cpp
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

This file both moves and changes things. Please do the move in a separate patch so I can review the move separate from the change.

@@ +29,5 @@
> +        return;
> +    }
> +
> +    if (buffer) {
> +        if ((buffer->Target() != LOCAL_GL_NONE) && (target != buffer->Target()))

Consider:
// Target valid if buffer target is unbound, or already bound to target.
bool isValidTargetForBuffer = !buffer->Target() || target == buffer->Target();
if (!isValidTargetForBuffer)

@@ +70,5 @@
> +    GetBufferByTargetIndexed(target, index) = buffer;
> +
> +    MakeContextCurrent();
> +
> +    gl->fBindBufferBase(target, index, buffer ? buffer->GLName() : 0);

We need to validate that `index` is in-range.

@@ +425,5 @@
> +    }
> +
> +    MOZ_CRASH("private WebGLContext::GetBufferByTarget: unknown target");
> +
> +    return mUnknownBufferTarget;

It would be much safer to return a pointer, so we could return null in this case. This looks like it should be fine, but it's not confidence-inducing.
We should generate INVALID_VALUE in this failure case, but assert that we can't get here on DEBUG builds.

::: dom/base/nsWrapperCache.h
@@ +613,5 @@
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END                         \
>    NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(_class)
>  
> +#define NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_11(_class, _field1,\
> +                                                 _field2,       \

Hah, this is getting ridiculous. Align _fieldN with _fieldN-1. Move _field1 to the next line if it helps.

@@ +638,5 @@
> +    NS_IMPL_CYCLE_COLLECTION_UNLINK(_field10)                   \
> +    NS_IMPL_CYCLE_COLLECTION_UNLINK(_field11)                   \
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER           \
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK_END                           \
> +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(_class)               \

Align the backslashes.

::: dom/webidl/WebGL2RenderingContext.webidl
@@ +73,5 @@
>      const GLenum RASTERIZER_DISCARD          = 0x8C89;
> +    const GLenum TRANSFORM_FEEDBACK_BUFFER   = 0x8C8E;
> +    const GLenum TRANSFORM_FEEDBACK_BUFFER_BINDING = 0x8C8F;
> +    const GLenum TRANSFORM_FEEDBACK_BUFFER_START = 0x8C84;
> +    const GLenum TRANSFORM_FEEDBACK_BUFFER_SIZE = 0x8C85;

Line the '='s up.

@@ +82,5 @@
> +                         GLintptr offset, GLsizeiptr size);
> +
> +    /* state requests */
> +    [Throws]
> +    any getParameterIndexed(GLenum pname, GLuint index);

Why does this throw?

::: gfx/gl/GLContext.cpp
@@ +674,5 @@
> +
> +            if (!LoadSymbols(indexedGetterSymbols, trygl, prefix)) {
> +                NS_ERROR("GL supports indexed integer getter without supplying it function.");
> +
> +                mInitialized &= MarkExtensionGroupUnsupported(XXX_indexed_integer_getter);

Don't fail init when optional symbols fail to load.

::: gfx/gl/GLContextExtensionGroupQueries.cpp
@@ +73,5 @@
>              GLContext::Extensions_End
>          }
>      },
>      {
> +        "XXX_indexed_integer_getter",

get_integer_indexed
I don't think we should bother with this, though. This feature isn't useful to us, since we actually only care about *_transform_feedback. It's not useful to separate this from the transform_feedback logic.

Add it later when it becomes useful.

::: gfx/gl/GLContextSymbols.h
@@ +422,5 @@
> +
> +    // EXT_draw_buffers2
> +    // fGetIntegeri_v == fGetIntegerIndexedv
> +    typedef void (GLAPIENTRY * PFNGLGETINTEGERI_V) (GLenum param, GLuint index, GLint* values);
> +    PFNGLGETINTEGERI_V fGetIntegeri_v;

Unusual that they would rename the function in GLES, but I agree with using the newer name.
Let's pull directly from GLES3 then, and s/values/data/.

However, I would not separate this out into EXT_draw_buffers2 unless we wanted to actually support EXT_draw_buffers2, or EXT_transform_feedback claimed it as a dependency. (which it doesn't actually. It just says that these functions are copied from draw_buffers2)

Let's pretend draw_buffers2 doesn't exist until we actually want to care about it. Just put all of the funcs listed in transform_feedback down below.

@@ +428,5 @@
> +    // EXT_transform_feedback
> +    typedef void (GLAPIENTRY * PFNGLBINDBUFFERBASE) (GLenum target, GLuint index, GLuint buffer);
> +    PFNGLBINDBUFFERBASE fBindBufferBase;
> +    typedef void (GLAPIENTRY * PFNGLBINDBUFFERRANGE) (GLenum target, GLuint index, GLuint buffer, GLintptr offset, GLsizeiptr size);
> +    PFNGLBINDBUFFERRANGE fBindBufferRange;

We need to add all the entry points when we add an extension.

Comment 3

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 788398 [details] [diff] [review]
> patch revision 1
> 
> Review of attachment 788398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If we don't address things like `index` bounds checking now, we're likely to
> forget in the future.
> 
> ::: content/canvas/src/WebGLContextBuffers.cpp
> @@ +1,5 @@
> > +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> 
> This file both moves and changes things. Please do the move in a separate
> patch so I can review the move separate from the change.
Oups...
> 
> @@ +29,5 @@
> > +        return;
> > +    }
> > +
> > +    if (buffer) {
> > +        if ((buffer->Target() != LOCAL_GL_NONE) && (target != buffer->Target()))
> 
> Consider:
> // Target valid if buffer target is unbound, or already bound to target.
> bool isValidTargetForBuffer = !buffer->Target() || target ==
> buffer->Target();
> if (!isValidTargetForBuffer)
Fixed.
> 
> @@ +70,5 @@
> > +    GetBufferByTargetIndexed(target, index) = buffer;
> > +
> > +    MakeContextCurrent();
> > +
> > +    gl->fBindBufferBase(target, index, buffer ? buffer->GLName() : 0);
> 
> We need to validate that `index` is in-range.
ValidateBufferTargetIndexed is already disigned for that.
> 
> @@ +425,5 @@
> > +    }
> > +
> > +    MOZ_CRASH("private WebGLContext::GetBufferByTarget: unknown target");
> > +
> > +    return mUnknownBufferTarget;
> 
> It would be much safer to return a pointer, so we could return null in this
> case. This looks like it should be fine, but it's not confidence-inducing.
> We should generate INVALID_VALUE in this failure case, but assert that we
> can't get here on DEBUG builds.
We would crash anyway before. I did that just for compile time errors.
> 
> ::: dom/base/nsWrapperCache.h
> @@ +613,5 @@
> >    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END                         \
> >    NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(_class)
> >  
> > +#define NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_11(_class, _field1,\
> > +                                                 _field2,       \
> 
> Hah, this is getting ridiculous. Align _fieldN with _fieldN-1. Move _field1
> to the next line if it helps.
Yeah, It was like that on others. Fixed.
> 
> @@ +638,5 @@
> > +    NS_IMPL_CYCLE_COLLECTION_UNLINK(_field10)                   \
> > +    NS_IMPL_CYCLE_COLLECTION_UNLINK(_field11)                   \
> > +  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER           \
> > +  NS_IMPL_CYCLE_COLLECTION_UNLINK_END                           \
> > +    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(_class)               \
> 
> Align the backslashes.
Fixed!
> 
> ::: dom/webidl/WebGL2RenderingContext.webidl
> @@ +73,5 @@
> >      const GLenum RASTERIZER_DISCARD          = 0x8C89;
> > +    const GLenum TRANSFORM_FEEDBACK_BUFFER   = 0x8C8E;
> > +    const GLenum TRANSFORM_FEEDBACK_BUFFER_BINDING = 0x8C8F;
> > +    const GLenum TRANSFORM_FEEDBACK_BUFFER_START = 0x8C84;
> > +    const GLenum TRANSFORM_FEEDBACK_BUFFER_SIZE = 0x8C85;
> 
> Line the '='s up.
Fixed!
> 
> @@ +82,5 @@
> > +                         GLintptr offset, GLsizeiptr size);
> > +
> > +    /* state requests */
> > +    [Throws]
> > +    any getParameterIndexed(GLenum pname, GLuint index);
> 
> Why does this throw?
Same as getParameter, but with an index.
> 
> ::: gfx/gl/GLContext.cpp
> @@ +674,5 @@
> > +
> > +            if (!LoadSymbols(indexedGetterSymbols, trygl, prefix)) {
> > +                NS_ERROR("GL supports indexed integer getter without supplying it function.");
> > +
> > +                mInitialized &= MarkExtensionGroupUnsupported(XXX_indexed_integer_getter);
> 
> Don't fail init when optional symbols fail to load.
It would not, because mGLVersion <= 210 without the GL_VERSION parser, that  is depending on the patch bringing cache for feature and solve all thoses problem at same time.
> 
> ::: gfx/gl/GLContextExtensionGroupQueries.cpp
> @@ +73,5 @@
> >              GLContext::Extensions_End
> >          }
> >      },
> >      {
> > +        "XXX_indexed_integer_getter",
> 
> get_integer_indexed
> I don't think we should bother with this, though. This feature isn't useful
> to us, since we actually only care about *_transform_feedback. It's not
> useful to separate this from the transform_feedback logic.
> 
> Add it later when it becomes useful.
It is because it's added in 2 extension actualy, _transform_feedback and _draw_buffers2
> 
> ::: gfx/gl/GLContextSymbols.h
> @@ +422,5 @@
> > +
> > +    // EXT_draw_buffers2
> > +    // fGetIntegeri_v == fGetIntegerIndexedv
> > +    typedef void (GLAPIENTRY * PFNGLGETINTEGERI_V) (GLenum param, GLuint index, GLint* values);
> > +    PFNGLGETINTEGERI_V fGetIntegeri_v;
> 
> Unusual that they would rename the function in GLES, but I agree with using
> the newer name.
> Let's pull directly from GLES3 then, and s/values/data/.
Fixed.
> 
> However, I would not separate this out into EXT_draw_buffers2 unless we
> wanted to actually support EXT_draw_buffers2, or EXT_transform_feedback
> claimed it as a dependency. (which it doesn't actually. It just says that
> these functions are copied from draw_buffers2)
> 
> Let's pretend draw_buffers2 doesn't exist until we actually want to care
> about it. Just put all of the funcs listed in transform_feedback down below.
I don't like that because :
(Note: These indexed query functions are provided in the EXT_draw_buffers2
extension.  The boolean query is not useful for any queryable value in
this extension, but is supported for completeness and consistency with
base GL typed "Get" functions.)
in http://www.opengl.org/registry/specs/EXT/transform_feedback.txt
> 
> @@ +428,5 @@
> > +    // EXT_transform_feedback
> > +    typedef void (GLAPIENTRY * PFNGLBINDBUFFERBASE) (GLenum target, GLuint index, GLuint buffer);
> > +    PFNGLBINDBUFFERBASE fBindBufferBase;
> > +    typedef void (GLAPIENTRY * PFNGLBINDBUFFERRANGE) (GLenum target, GLuint index, GLuint buffer, GLintptr offset, GLsizeiptr size);
> > +    PFNGLBINDBUFFERRANGE fBindBufferRange;
> 
> We need to add all the entry points when we add an extension.
Fixed.

Comment 4

[Guillaume Abadie]

Attachment: patch revision 2 - step 1: move buffer stuff in WebGLContextBuffers.cpp

Comment 5

[Guillaume Abadie]

Attachment: patch revision 3 - step 1: move buffer stuff in WebGLContextBuffers.cpp

Comment 6

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

Comment on attachment 791292 [details] [diff] [review]
patch revision 3 - step 1: move buffer stuff in WebGLContextBuffers.cpp

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

AIUI the basic design around here is already adopted in e.g. WebGLContextFramebufferOperations.cpp, and this is basically the same pattern, so r+.

Comment 7

[Guillaume Abadie]

Attachment: patch step 2 revision 1: Add transform feedback symbols in GLContext

Comment 8

[Guillaume Abadie]

Attachment: patch step 3 revision 1: Add TRANSFORM_FEEDBACK_BUFFER in WebGL2

Implement TRANSFORM_FEEDBACK_BUFFER in WebGL 2

Comment 9

[Guillaume Abadie]

Attachment: 791414: patch step 3 revision 2: Add TRANSFORM_FEEDBACK_BUFFER in WebGL2

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 791368 [details] [diff] [review]
patch step 2 revision 1: Add transform feedback symbols in GLContext

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

Since GLES3 doesn't seem to support an equivalent to draw_buffers2, just forget that it exists. There's no reason to track it, so just treat it like transform_feedback originated the GetIntegerIndexed funcs.

::: gfx/gl/GLContext.cpp
@@ +657,5 @@
>  
> +        if (IsExtensionSupported(XXX_get_integer_indexed)) {
> +            SymLoadStruct indexedGetterSymbols[] = {
> +                { (PRFuncPtr*) &mSymbols.fGetIntegeri_v,
> +                  { "fGetIntegeri_v",

No 'f' prefix on GL symbols.

@@ +667,5 @@
> +                { nullptr, { nullptr } },
> +            };
> +
> +            if (!LoadSymbols(indexedGetterSymbols, trygl, prefix)) {
> +                NS_ERROR("GL supports indexed integer getter without supplying it function.");

s/it/its/

@@ +722,5 @@
> +                { nullptr, { nullptr } },
> +            };
> +
> +            if (!LoadSymbols(transformFeedbackSymbols, trygl, prefix)) {
> +                NS_ERROR("GL supports transform feedback without supplying it functions.");

s/it/its/

@@ +748,5 @@
> +                { nullptr, { nullptr } },
> +            };
> +
> +            if (!LoadSymbols(bindBufferOffsetSymbols, trygl, prefix)) {
> +                NS_ERROR("GL supports transform feedback without supplying it functions.");

s/transform feedback/BindBufferOffset/
s/it functions/its function/

::: gfx/gl/GLContext.h
@@ +368,5 @@
>          ARB_vertex_array_object,
>          APPLE_vertex_array_object,
>          ARB_draw_buffers,
>          EXT_draw_buffers,
> +        EXT_draw_buffers2,

Don't add an extension unless you add all its funcs.

::: gfx/gl/GLContextExtensionGroupQueries.cpp
@@ +126,5 @@
>              GLContext::Extensions_End
>          }
>      },
>      {
> +        "XXX_get_integer_indexed",

I still think this is overkill for making a group for. We don't use this func except with transform_feedback, which, while it notes that the functions were first introduced in draw_buffers2, doesn't require draw_buffers2. There's no reason yet to load this symbol if we have draw_buffers2 but not transform_feedback, thus no need for this being a separate group.

Groups are supposed to make things easier, not more complicated.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 791429 [details] [diff] [review]
791414: patch step 3 revision 2: Add TRANSFORM_FEEDBACK_BUFFER in WebGL2

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

::: content/canvas/src/WebGLContext.h
@@ +771,2 @@
>      bool ValidateBufferTarget(GLenum target, const char* infos);
> +    bool ValidateBufferTargetIndexed(GLenum target, GLuint index, const char* infos);

I'm sure this is `infos` and not `funcName` for hysterical raisins. Just a curiosity. *shrug*

::: content/canvas/src/WebGLContextBuffers.cpp
@@ +35,5 @@
>          buffer->SetTarget(target);
>          buffer->SetHasEverBeenBound(true);
>      }
>  
> +    GetBufferByTarget(target) = buffer;

if (!SetBufferByTarget(target, buffer)) {
  MOZ_ASSERT(false, "SetBufferByTarget failed. Check `target` validation.");
  return ErrorInvalidEnum("bindBuffer: Invalid `target`.");
}

@@ +64,5 @@
> +    if (buffer) {
> +        if ((buffer->Target() != LOCAL_GL_NONE) && (target != buffer->Target()))
> +            return ErrorInvalidOperation("BindBufferBase: buffer already bound to a different target");
> +        buffer->SetTarget(target);
> +        buffer->SetHasEverBeenBound(true);

Consider:
if (!buffer->Target()) {
  buffer->SetTarget(target);
  buffer->SetHasEverBeenBound(true);
} else if (target != buffer->Target()) {
  return ErrorInvalidOperation(...);
}

@@ +162,5 @@
>          // see http://www.khronos.org/bugzilla/show_bug.cgi?id=386
>          return ErrorInvalidValue("bufferData: null object passed");
>      }
>  
> +    if (!ValidateBufferTarget(target, "bufferData")){

Space between `)` and `{`. ([1])

@@ +199,5 @@
>  {
>      if (!IsContextStable())
>          return;
>  
> +    if (!ValidateBufferTarget(target, "bufferData")){

[1]

@@ +238,5 @@
>          // see http://www.khronos.org/bugzilla/show_bug.cgi?id=386
>          return;
>      }
>  
> +    if (!ValidateBufferTarget(target, "bufferSubData")){

[1]

@@ +274,5 @@
>  {
>      if (!IsContextStable())
>          return;
>  
> +    if (!ValidateBufferTarget(target, "bufferSubData")){

[1]

@@ +359,5 @@
>      {
>          case LOCAL_GL_ARRAY_BUFFER:
>          case LOCAL_GL_ELEMENT_ARRAY_BUFFER:
>              return true;
> +            

whitespace here, and in two places below.

@@ +377,5 @@
> +
> +bool
> +WebGLContext::ValidateBufferTargetIndexed(WebGLenum target, GLuint index, const char* infos)
> +{
> +    MOZ_ASSERT(mGLMaxTransformFeedbackSeparateAttribs > 0, "mGLMaxTransformFeedbackSeparateAttribs must be positive.");

Just make this var a uint so we don't do this everywhere.

@@ +387,5 @@
> +                ErrorInvalidValue("%s: index should be less than MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS", infos, index);
> +                return false;
> +            }
> +            break;// See bug 903594
> +            return true;

Don't leave dead code.

@@ +432,5 @@
> +        default:
> +            break;
> +    }
> +
> +    MOZ_CRASH("private WebGLContext::GetBufferByTarget: corrupted buffer object target.");

We shouldn't use MOZ_CRASH unless there's nothing we can do to recover.

We can recover more gracefully than crashing:
If GetBufferByTarget[Indexed] fails, emit INVALID_ENUM.

Yes, this should already be checked for at the moment, but we don't want to accidentally let people crash get DoS'd if we mess up a switch statement somewhere.

@@ +434,5 @@
> +    }
> +
> +    MOZ_CRASH("private WebGLContext::GetBufferByTarget: corrupted buffer object target.");
> +
> +    return mUnknownBufferTarget;

IIRC, MOZ_CRASH is no-return, anyways.

@@ +438,5 @@
> +    return mUnknownBufferTarget;
> +}
> +
> +WebGLRefPtr<WebGLBuffer>&
> +WebGLContext::GetBufferByTargetIndexed(GLenum target, GLuint index)

Same complaints as for GetBufferByTarget.

::: content/canvas/src/WebGLContextState.cpp
@@ +497,5 @@
> +
> +    switch (pname) {
> +        case LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER_BINDING:
> +        {
> +            return WebGLObjectAsJSValue(cx, (WebGLBuffer*)nullptr, rv);// See bug 903594

I don't like leaving things like this broken. Just let this fall through to INVALID_ENUM until we actually fix this.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +906,5 @@
>                      break;
>                  default:
>                      GenerateWarning("GL error 0x%x occurred during WebGL context initialization!", error);
>                      return false;
>              }   

whitespace

@@ +914,5 @@
> +    if (IsWebGL2()) {
> +        gl->fGetIntegerv(LOCAL_GL_MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS, &mGLMaxTransformFeedbackSeparateAttribs);
> +
> +        if (mGLMaxTransformFeedbackSeparateAttribs <= 0){
> +            GenerateWarning("MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS is supposed to be positive!");

Assert this, don't warn.
Unless there's a spec which allows negative numbers to come out of this, assume it's non-negative. 
You can use gl->GetUInteger, as well.

GetUInteger should actually probably assert non-negative in debug builds.

::: dom/webidl/WebGL2RenderingContext.webidl
@@ +71,5 @@
>  
>      /* transform feedback */
>      const GLenum RASTERIZER_DISCARD          = 0x8C89;
> +    const GLenum TRANSFORM_FEEDBACK_BUFFER   = 0x8C8E;
> +    const GLenum TRANSFORM_FEEDBACK_BUFFER_BINDING   = 0x8C8F;

Just align all of these '='s for this section.

@@ +83,5 @@
> +                         GLintptr offset, GLsizeiptr size);
> +
> +    /* state requests */
> +    [Throws]
> +    any getParameterIndexed(GLenum pname, GLuint index);

Don't mark it Throws unless it actually throws. (It doesn't)

Comment 12

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> Comment on attachment 791368 [details] [diff] [review]
> patch step 2 revision 1: Add transform feedback symbols in GLContext
> 
> Review of attachment 791368 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since GLES3 doesn't seem to support an equivalent to draw_buffers2, just
> forget that it exists. There's no reason to track it, so just treat it like
> transform_feedback originated the GetIntegerIndexed funcs.
> 
> ::: gfx/gl/GLContext.cpp
> @@ +657,5 @@
> >  
> > +        if (IsExtensionSupported(XXX_get_integer_indexed)) {
> > +            SymLoadStruct indexedGetterSymbols[] = {
> > +                { (PRFuncPtr*) &mSymbols.fGetIntegeri_v,
> > +                  { "fGetIntegeri_v",
> 
> No 'f' prefix on GL symbols.
> 
> @@ +667,5 @@
> > +                { nullptr, { nullptr } },
> > +            };
> > +
> > +            if (!LoadSymbols(indexedGetterSymbols, trygl, prefix)) {
> > +                NS_ERROR("GL supports indexed integer getter without supplying it function.");
> 
> s/it/its/
> 
> @@ +722,5 @@
> > +                { nullptr, { nullptr } },
> > +            };
> > +
> > +            if (!LoadSymbols(transformFeedbackSymbols, trygl, prefix)) {
> > +                NS_ERROR("GL supports transform feedback without supplying it functions.");
> 
> s/it/its/
> 
> @@ +748,5 @@
> > +                { nullptr, { nullptr } },
> > +            };
> > +
> > +            if (!LoadSymbols(bindBufferOffsetSymbols, trygl, prefix)) {
> > +                NS_ERROR("GL supports transform feedback without supplying it functions.");
> 
> s/transform feedback/BindBufferOffset/
> s/it functions/its function/
> 
> ::: gfx/gl/GLContext.h
> @@ +368,5 @@
> >          ARB_vertex_array_object,
> >          APPLE_vertex_array_object,
> >          ARB_draw_buffers,
> >          EXT_draw_buffers,
> > +        EXT_draw_buffers2,
> 
> Don't add an extension unless you add all its funcs.
> 
> ::: gfx/gl/GLContextExtensionGroupQueries.cpp
> @@ +126,5 @@
> >              GLContext::Extensions_End
> >          }
> >      },
> >      {
> > +        "XXX_get_integer_indexed",
> 
> I still think this is overkill for making a group for. We don't use this
> func except with transform_feedback, which, while it notes that the
> functions were first introduced in draw_buffers2, doesn't require
> draw_buffers2. There's no reason yet to load this symbol if we have
> draw_buffers2 but not transform_feedback, thus no need for this being a
> separate group.
> 
> Groups are supposed to make things easier, not more complicated.
All fixed.

Comment 13

[Guillaume Abadie]

Attachment: patch step 2 - revision 2: Add transform feedback symbols in GLContext

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 791552 [details] [diff] [review]
patch step 2 - revision 2: Add transform feedback symbols in GLContext

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

::: gfx/gl/GLContextSymbols.h
@@ +419,5 @@
>      // ARB_instanced_array
>      typedef void (GLAPIENTRY * PFNGLVERTEXATTRIBDIVISOR) (GLuint index, GLuint divisor);
>      PFNGLVERTEXATTRIBDIVISOR fVertexAttribDivisor;
> +
> +    // EXT_draw_buffers2 / OpenGL (ES) 3.0

Put this in the transform_feedback func section.

Comment 15

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Comment on attachment 791552 [details] [diff] [review]
> patch step 2 - revision 2: Add transform feedback symbols in GLContext
> 
> Review of attachment 791552 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContextSymbols.h
> @@ +419,5 @@
> >      // ARB_instanced_array
> >      typedef void (GLAPIENTRY * PFNGLVERTEXATTRIBDIVISOR) (GLuint index, GLuint divisor);
> >      PFNGLVERTEXATTRIBDIVISOR fVertexAttribDivisor;
> > +
> > +    // EXT_draw_buffers2 / OpenGL (ES) 3.0
> 
> Put this in the transform_feedback func section.
Oups... Fixed!
Thanks!

Comment 16

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #11)
> Comment on attachment 791429 [details] [diff] [review]
> 791414: patch step 3 revision 2: Add TRANSFORM_FEEDBACK_BUFFER in WebGL2
> 
> Review of attachment 791429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.h
> @@ +771,2 @@
> >      bool ValidateBufferTarget(GLenum target, const char* infos);
> > +    bool ValidateBufferTargetIndexed(GLenum target, GLuint index, const char* infos);
> 
> I'm sure this is `infos` and not `funcName` for hysterical raisins. Just a
> curiosity. *shrug*
Even if I agree, We have always used to call it like that for all Validate functions.
> 
> ::: content/canvas/src/WebGLContextBuffers.cpp
> @@ +35,5 @@
> >          buffer->SetTarget(target);
> >          buffer->SetHasEverBeenBound(true);
> >      }
> >  
> > +    GetBufferByTarget(target) = buffer;
> 
> if (!SetBufferByTarget(target, buffer)) {
>   MOZ_ASSERT(false, "SetBufferByTarget failed. Check `target` validation.");
>   return ErrorInvalidEnum("bindBuffer: Invalid `target`.");
> }
Fixed.
> 
> @@ +64,5 @@
> > +    if (buffer) {
> > +        if ((buffer->Target() != LOCAL_GL_NONE) && (target != buffer->Target()))
> > +            return ErrorInvalidOperation("BindBufferBase: buffer already bound to a different target");
> > +        buffer->SetTarget(target);
> > +        buffer->SetHasEverBeenBound(true);
> 
> Consider:
> if (!buffer->Target()) {
>   buffer->SetTarget(target);
>   buffer->SetHasEverBeenBound(true);
> } else if (target != buffer->Target()) {
>   return ErrorInvalidOperation(...);
> }
That was same as in BindBuffer. I will fix that too.
> 
> @@ +162,5 @@
> >          // see http://www.khronos.org/bugzilla/show_bug.cgi?id=386
> >          return ErrorInvalidValue("bufferData: null object passed");
> >      }
> >  
> > +    if (!ValidateBufferTarget(target, "bufferData")){
> 
> Space between `)` and `{`. ([1])
Oups... Fixed!
> 
> @@ +199,5 @@
> >  {
> >      if (!IsContextStable())
> >          return;
> >  
> > +    if (!ValidateBufferTarget(target, "bufferData")){
> 
> [1]
Oups... Fixed!
> 
> @@ +238,5 @@
> >          // see http://www.khronos.org/bugzilla/show_bug.cgi?id=386
> >          return;
> >      }
> >  
> > +    if (!ValidateBufferTarget(target, "bufferSubData")){
> 
> [1]
Oups... Fixed!
> 
> @@ +274,5 @@
> >  {
> >      if (!IsContextStable())
> >          return;
> >  
> > +    if (!ValidateBufferTarget(target, "bufferSubData")){
> 
> [1]
Oups... Fixed!
> 
> @@ +359,5 @@
> >      {
> >          case LOCAL_GL_ARRAY_BUFFER:
> >          case LOCAL_GL_ELEMENT_ARRAY_BUFFER:
> >              return true;
> > +            
> 
> whitespace here, and in two places below.
Oups... Fixed!
> 
> @@ +377,5 @@
> > +
> > +bool
> > +WebGLContext::ValidateBufferTargetIndexed(WebGLenum target, GLuint index, const char* infos)
> > +{
> > +    MOZ_ASSERT(mGLMaxTransformFeedbackSeparateAttribs > 0, "mGLMaxTransformFeedbackSeparateAttribs must be positive.");
> 
> Just make this var a uint so we don't do this everywhere.
Fixed.
> 
> @@ +387,5 @@
> > +                ErrorInvalidValue("%s: index should be less than MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS", infos, index);
> > +                return false;
> > +            }
> > +            break;// See bug 903594
> > +            return true;
> 
> Don't leave dead code.
Fixed!
> 
> @@ +432,5 @@
> > +        default:
> > +            break;
> > +    }
> > +
> > +    MOZ_CRASH("private WebGLContext::GetBufferByTarget: corrupted buffer object target.");
> 
> We shouldn't use MOZ_CRASH unless there's nothing we can do to recover.
> 
> We can recover more gracefully than crashing:
> If GetBufferByTarget[Indexed] fails, emit INVALID_ENUM.
> 
> Yes, this should already be checked for at the moment, but we don't want to
> accidentally let people crash get DoS'd if we mess up a switch statement
> somewhere.
Fixed.
> 
> @@ +434,5 @@
> > +    }
> > +
> > +    MOZ_CRASH("private WebGLContext::GetBufferByTarget: corrupted buffer object target.");
> > +
> > +    return mUnknownBufferTarget;
> 
> IIRC, MOZ_CRASH is no-return, anyways.
I hope so! But we have to keep something here to prevent compile-time errors.
> 
> @@ +438,5 @@
> > +    return mUnknownBufferTarget;
> > +}
> > +
> > +WebGLRefPtr<WebGLBuffer>&
> > +WebGLContext::GetBufferByTargetIndexed(GLenum target, GLuint index)
> 
> Same complaints as for GetBufferByTarget.
Fixed.
> 
> ::: content/canvas/src/WebGLContextState.cpp
> @@ +497,5 @@
> > +
> > +    switch (pname) {
> > +        case LOCAL_GL_TRANSFORM_FEEDBACK_BUFFER_BINDING:
> > +        {
> > +            return WebGLObjectAsJSValue(cx, (WebGLBuffer*)nullptr, rv);// See bug 903594
> 
> I don't like leaving things like this broken. Just let this fall through to
> INVALID_ENUM until we actually fix this.
Fair enough! Fixed.
> 
> ::: content/canvas/src/WebGLContextValidate.cpp
> @@ +906,5 @@
> >                      break;
> >                  default:
> >                      GenerateWarning("GL error 0x%x occurred during WebGL context initialization!", error);
> >                      return false;
> >              }   
> 
> whitespace
Oups...
> 
> @@ +914,5 @@
> > +    if (IsWebGL2()) {
> > +        gl->fGetIntegerv(LOCAL_GL_MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS, &mGLMaxTransformFeedbackSeparateAttribs);
> > +
> > +        if (mGLMaxTransformFeedbackSeparateAttribs <= 0){
> > +            GenerateWarning("MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS is supposed to be positive!");
> 
> Assert this, don't warn.
Fixed!
> Unless there's a spec which allows negative numbers to come out of this,
> assume it's non-negative. 
> You can use gl->GetUInteger, as well.
You are right, but GetUInteger is not standard.
> 
> GetUInteger should actually probably assert non-negative in debug builds.
No, It doesn't.
Fixed.
> 
> ::: dom/webidl/WebGL2RenderingContext.webidl
> @@ +71,5 @@
> >  
> >      /* transform feedback */
> >      const GLenum RASTERIZER_DISCARD          = 0x8C89;
> > +    const GLenum TRANSFORM_FEEDBACK_BUFFER   = 0x8C8E;
> > +    const GLenum TRANSFORM_FEEDBACK_BUFFER_BINDING   = 0x8C8F;
> 
> Just align all of these '='s for this section.
Fixed!
> 
> @@ +83,5 @@
> > +                         GLintptr offset, GLsizeiptr size);
> > +
> > +    /* state requests */
> > +    [Throws]
> > +    any getParameterIndexed(GLenum pname, GLuint index);
> 
> Don't mark it Throws unless it actually throws. (It doesn't)
Oups... Fixed!

Comment 17

[Guillaume Abadie]

Attachment: patch step 3 - revision 3: Add TRANSFORM_FEEDBACK_BUFFER in WebGL2

Comment 18

[Guillaume Abadie]

Attachment: patch step 3 - revision 4: Add TRANSFORM_FEEDBACK_BUFFER in WebGL2

Trailing white-spaces fix.

Comment 19

[Kelsey Gilbert [:jgilbert]]

(In reply to Guillaume Abadie from comment #16)
> (In reply to Jeff Gilbert [:jgilbert] from comment #11)
> > Comment on attachment 791429 [details] [diff] [review]
> > 791414: patch step 3 revision 2: Add TRANSFORM_FEEDBACK_BUFFER in WebGL2
> > 
> > Review of attachment 791429 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ +434,5 @@
> > > +    }
> > > +
> > > +    MOZ_CRASH("private WebGLContext::GetBufferByTarget: corrupted buffer object target.");
> > > +
> > > +    return mUnknownBufferTarget;
> > 
> > IIRC, MOZ_CRASH is no-return, anyways.
> I hope so! But we have to keep something here to prevent compile-time errors.
I believe the point of no-return is that the compiler won't complain about not having a return value, so you shouldn't be seeing any compile warnings or errors here anyways.
> > @@ +914,5 @@
> > > +    if (IsWebGL2()) {
> > > +        gl->fGetIntegerv(LOCAL_GL_MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS, &mGLMaxTransformFeedbackSeparateAttribs);
> > > +
> > > +        if (mGLMaxTransformFeedbackSeparateAttribs <= 0){
> > > +            GenerateWarning("MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS is supposed to be positive!");
> > 
> > Assert this, don't warn.
> Fixed!
> > Unless there's a spec which allows negative numbers to come out of this,
> > assume it's non-negative. 
> > You can use gl->GetUInteger, as well.
> You are right, but GetUInteger is not standard.
It's true, but this comes up so often we added a function to handle it.

Comment 20

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #19)
> (In reply to Guillaume Abadie from comment #16)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #11)
> > > Comment on attachment 791429 [details] [diff] [review]
> > > 791414: patch step 3 revision 2: Add TRANSFORM_FEEDBACK_BUFFER in WebGL2
> > > 
> > > Review of attachment 791429 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > @@ +434,5 @@
> > > > +    }
> > > > +
> > > > +    MOZ_CRASH("private WebGLContext::GetBufferByTarget: corrupted buffer object target.");
> > > > +
> > > > +    return mUnknownBufferTarget;
> > > 
> > > IIRC, MOZ_CRASH is no-return, anyways.
> > I hope so! But we have to keep something here to prevent compile-time errors.
> I believe the point of no-return is that the compiler won't complain about
> not having a return value, so you shouldn't be seeing any compile warnings
> or errors here anyways.
I switch like we talked Friday night. So the new revision is different.
> > > @@ +914,5 @@
> > > > +    if (IsWebGL2()) {
> > > > +        gl->fGetIntegerv(LOCAL_GL_MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS, &mGLMaxTransformFeedbackSeparateAttribs);
> > > > +
> > > > +        if (mGLMaxTransformFeedbackSeparateAttribs <= 0){
> > > > +            GenerateWarning("MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS is supposed to be positive!");
> > > 
> > > Assert this, don't warn.
> > Fixed!
> > > Unless there's a spec which allows negative numbers to come out of this,
> > > assume it's non-negative. 
> > > You can use gl->GetUInteger, as well.
> > You are right, but GetUInteger is not standard.
> It's true, but this comes up so often we added a function to handle it.
I used it in the new revisions. =)

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 791605 [details] [diff] [review]
patch step 3 - revision 4: Add TRANSFORM_FEEDBACK_BUFFER in WebGL2

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

::: content/canvas/src/WebGLContext.h
@@ +768,5 @@
> +    // TRANSFORM_FEEDBACK_BUFFER slot
> +    WebGLRefPtr<WebGLBuffer> mBoundTransformFeedbackBuffer;
> +
> +    WebGLRefPtr<WebGLBuffer>* GetBufferSlotByTarget(GLenum target, const char* infos);
> +    WebGLRefPtr<WebGLBuffer>* GetBufferSlotByTargetIndexed(GLenum target, GLuint index, const char* infos);

Add a comment that these two functions emit INVALID_ENUM for invalid `target`.

::: content/canvas/src/WebGLContextBuffers.cpp
@@ +135,5 @@
>  
>      if (!ValidateBufferUsageEnum(usage, "bufferData: usage"))
>          return;
>  
> +    WebGLBuffer* boundBuffer = bufferSlot->get();

You should also be able to just use `*bufferSlot`.

@@ +420,5 @@
> +            if (index >= mGLMaxTransformFeedbackSeparateAttribs) {
> +                ErrorInvalidValue("%s: index should be less than MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS", infos, index);
> +                return nullptr;
> +            }
> +            return nullptr;// See bug 903594

space before `//`

Comment 22

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #21)
> Comment on attachment 791605 [details] [diff] [review]
> patch step 3 - revision 4: Add TRANSFORM_FEEDBACK_BUFFER in WebGL2
> 
> Review of attachment 791605 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.h
> @@ +768,5 @@
> > +    // TRANSFORM_FEEDBACK_BUFFER slot
> > +    WebGLRefPtr<WebGLBuffer> mBoundTransformFeedbackBuffer;
> > +
> > +    WebGLRefPtr<WebGLBuffer>* GetBufferSlotByTarget(GLenum target, const char* infos);
> > +    WebGLRefPtr<WebGLBuffer>* GetBufferSlotByTargetIndexed(GLenum target, GLuint index, const char* infos);
> 
> Add a comment that these two functions emit INVALID_ENUM for invalid
> `target`.
Fixed.
> 
> ::: content/canvas/src/WebGLContextBuffers.cpp
> @@ +135,5 @@
> >  
> >      if (!ValidateBufferUsageEnum(usage, "bufferData: usage"))
> >          return;
> >  
> > +    WebGLBuffer* boundBuffer = bufferSlot->get();
> 
> You should also be able to just use `*bufferSlot`.
True but in my mind, this way is more readable and doesn't cost anything.
> 
> @@ +420,5 @@
> > +            if (index >= mGLMaxTransformFeedbackSeparateAttribs) {
> > +                ErrorInvalidValue("%s: index should be less than MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS", infos, index);
> > +                return nullptr;
> > +            }
> > +            return nullptr;// See bug 903594
> 
> space before `//`
Oups... Fixed!
Thanks!

Comment 23

[Guillaume Abadie]

step 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/c805788476ea
step 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/07a69fca7c14
step 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/5ddc04b616a9

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c805788476ea
https://hg.mozilla.org/mozilla-central/rev/07a69fca7c14
https://hg.mozilla.org/mozilla-central/rev/5ddc04b616a9