Bug 41300 – Implement OpenGL ES 2.0 semantics for vertex attribute 0

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

Vertex attribute 0 has special semantics in desktop OpenGL, but in OpenGL ES 2.0 it behaves the same as any other generic vertex attribute. The differences are as follows:

 - On desktop GL, if vertex attribute 0 is not enabled as an array, then no rendering results are produced.
 - On desktop GL, vertex attribute 0 does not have any persistent state -- that is, it can not be set to a constant value.

The WebGL working group decided during a recent face-to-face that the OpenGL ES 2.0 semantics are the best ones to implement because they are the most consistent and easiest to explain. This requires a certain amount of emulation to be performed, but is tractable.

[Zhenyao Mo]

Created attachment 60175 [details]
patch

[WebKit Review Bot]

Attachment 60175 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/html/canvas/WebGLProgram.cpp:89:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zhenyao Mo]

Created attachment 60179 [details]
revised patch: fix style issue

I really DISLIKE this (x == 0) has to be written as (!x).

Sometimes 0 is just a number instead of the equivalence of false/null.

[Zhenyao Mo]

Comments from Gregg Tavares <[e-mail]>

http://wkrietveld.appspot.com/41300/diff/1/8
File WebCore/html/canvas/WebGLRenderingContext.cpp (right):

http://wkrietveld.appspot.com/41300/diff/1/8#newcode1693
WebCore/html/canvas/WebGLRenderingContext.cpp:1693: if (!isGLES2Compliant() &&
!index && m_vertexAttribState[0].bufferBinding == m_vertexAttrib0Buffer
Is the check "m_vertexAttribState[0].bufferBinding == m_vertexAttrib0Buffer" 
valid? I know initially that's true but if I go

bindBuffer(ARRAY_BUFFER, someBuffer);
ctx.vertexAttribPointer(0, 4, ctx.FLOAT, FALSE, 0, 0);
ctx.deleteBuffer(ARRAY_BUFFER, someBuffer);

What will this return?

http://wkrietveld.appspot.com/41300/diff/1/8#newcode1714
WebCore/html/canvas/WebGLRenderingContext.cpp:1714: return
WebGLGetInfo(m_vertexAttribState[index].stride);
This stride is wrong. Below in VertexAttribPointer you store the validated
stride, not the user's stride.

http://wkrietveld.appspot.com/41300/diff/1/8#newcode3725
WebCore/html/canvas/WebGLRenderingContext.cpp:3725:
m_vertexAttribState[index].value[ii] = v[ii];
The spec says unset values are set to 0.0f except the last value which is set to
1.0f

That means this should probably be add

before this loop OR do something after to make unset values get set back to the
correct values.
m_vertexAttribState[index].value[0] = 0.0f;
m_vertexAttribState[index].value[1] = 0.0f;
m_vertexAttribState[index].value[2] = 0.0f;
m_vertexAttribState[index].value[3] = 1.0f;

same for the others.

[Zhenyao Mo]

(In reply to comment #4)
> Comments from Gregg Tavares <[e-mail]>
> 
> http://wkrietveld.appspot.com/41300/diff/1/8
> File WebCore/html/canvas/WebGLRenderingContext.cpp (right):
> 
> http://wkrietveld.appspot.com/41300/diff/1/8#newcode1693
> WebCore/html/canvas/WebGLRenderingContext.cpp:1693: if (!isGLES2Compliant() &&
> !index && m_vertexAttribState[0].bufferBinding == m_vertexAttrib0Buffer
> Is the check "m_vertexAttribState[0].bufferBinding == m_vertexAttrib0Buffer" 
> valid? I know initially that's true but if I go
> 
> bindBuffer(ARRAY_BUFFER, someBuffer);
> ctx.vertexAttribPointer(0, 4, ctx.FLOAT, FALSE, 0, 0);
> ctx.deleteBuffer(ARRAY_BUFFER, someBuffer);
> 
> What will this return?

So in this case, there is another condition says !m_vertexAttribState[index].bufferBinding->object(), which will be true if the buffer has been deleted.

Will fix the rest.

> 
> http://wkrietveld.appspot.com/41300/diff/1/8#newcode1714
> WebCore/html/canvas/WebGLRenderingContext.cpp:1714: return
> WebGLGetInfo(m_vertexAttribState[index].stride);
> This stride is wrong. Below in VertexAttribPointer you store the validated
> stride, not the user's stride.
> 
> http://wkrietveld.appspot.com/41300/diff/1/8#newcode3725
> WebCore/html/canvas/WebGLRenderingContext.cpp:3725:
> m_vertexAttribState[index].value[ii] = v[ii];
> The spec says unset values are set to 0.0f except the last value which is set to
> 1.0f
> 
> That means this should probably be add
> 
> before this loop OR do something after to make unset values get set back to the
> correct values.
> m_vertexAttribState[index].value[0] = 0.0f;
> m_vertexAttribState[index].value[1] = 0.0f;
> m_vertexAttribState[index].value[2] = 0.0f;
> m_vertexAttribState[index].value[3] = 1.0f;
> 
> same for the others.

[Zhenyao Mo]

Created attachment 60189 [details]
revised patch: responding to gman's review

Revised according to Gregg's review.  Also add some code in deleteBuffer() to reset the vertex attrib 0 states if the deleted buffer was its current binding buffer.

[Gregg Tavares]

LGTM

[Zhenyao Mo]

Created attachment 60306 [details]
revised patch: add a new test gl-vertex-attrib.html

This patch is exactly the same as the previous one except the added test gl-vertex-attrib.html.

The test is copied from conformance tests.

It is necessary to add this test because it tests against the modifications this patch made.

[Kenneth Russell]

Generally looks good. A few issues.


WebCore/html/canvas/WebGLRenderingContext.cpp:3045
 +      if (index || isGLES2Compliant()) {
Throughout this patch I find this structure really confusing. Could you please restructure it like this:

if (!index && !isGLES2Compliant() {
  // vertex attrib 0 emulation code, likely containing early return.
}
// Normal case


WebCore/html/canvas/WebGLRenderingContext.cpp:3157
 +          m_context->synthesizeGLError(GraphicsContext3D::INVALID_ENUM);
This switch duplicates code -- you should use sizeInBytes to determine the element size and return early if the result is <= 0. (sizeInBytes already checks for and raises INVALID_ENUM.)


WebCore/html/canvas/WebGLRenderingContext.cpp:3729
 +          switch (size) {
You should be switching on expectedSize here.


WebCore/html/canvas/WebGLRenderingContext.cpp:3743
 +          cleanupAfterGraphicsCall(false);
Did you mean to return early here?

WebCore/html/canvas/WebGLRenderingContext.cpp:3748
 +      m_vertexAttribState[index].value[3] = 1.0f;
Can you factor this out into an "initValue()" method on the VertexAttribState class, and call that from the constructor as well?


WebCore/html/canvas/WebGLRenderingContext.h:373
 +                  value[3] = 1.0f;
Add and call "initValue()" instead.

[Zhenyao Mo]

(In reply to comment #9)
> Generally looks good. A few issues.
> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:3045
>  +      if (index || isGLES2Compliant()) {
> Throughout this patch I find this structure really confusing. Could you please restructure it like this:
> 
> if (!index && !isGLES2Compliant() {
>   // vertex attrib 0 emulation code, likely containing early return.
> }
> // Normal case

Actually the situation is the opposite: if it's vertex attrib 0 and it's GL, we skip the call of glVertexAttrib*().

I added comments above these logics and made it like this for readability:

if (index || (!index && isGLES2Compliant())

> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:3157
>  +          m_context->synthesizeGLError(GraphicsContext3D::INVALID_ENUM);
> This switch duplicates code -- you should use sizeInBytes to determine the element size and return early if the result is <= 0. (sizeInBytes already checks for and raises INVALID_ENUM.)

Done.

> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:3729
>  +          switch (size) {
> You should be switching on expectedSize here.

Done.

> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:3743
>  +          cleanupAfterGraphicsCall(false);
> Did you mean to return early here?

No.  Explained above.

> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:3748
>  +      m_vertexAttribState[index].value[3] = 1.0f;
> Can you factor this out into an "initValue()" method on the VertexAttribState class, and call that from the constructor as well?

Done.

> 
> 
> WebCore/html/canvas/WebGLRenderingContext.h:373
>  +                  value[3] = 1.0f;
> Add and call "initValue()" instead.

Done.

[Zhenyao Mo]

Created attachment 60315 [details]
revised patch : responding to Ken Russell's comments

[Kenneth Russell]

(In reply to comment #10)
> (In reply to comment #9)
> > Generally looks good. A few issues.
> > 
> > 
> > WebCore/html/canvas/WebGLRenderingContext.cpp:3045
> >  +      if (index || isGLES2Compliant()) {
> > Throughout this patch I find this structure really confusing. Could you please restructure it like this:
> > 
> > if (!index && !isGLES2Compliant() {
> >   // vertex attrib 0 emulation code, likely containing early return.
> > }
> > // Normal case
> 
> Actually the situation is the opposite: if it's vertex attrib 0 and it's GL, we skip the call of glVertexAttrib*().
> 
> I added comments above these logics and made it like this for readability:
> 
> if (index || (!index && isGLES2Compliant())

I understand what you're doing. I was suggesting to turn the logic around and put the fall-through case in the "if" clause. However I didn't realize you're always using the value in the VertexAttribState for getVertexAttrib. In that case the logic is cleaner the way you had it; sorry, but can you please put it back the way it was?

A couple more issues:

WebCore/ChangeLog:21
 +          (WebCore::WebGLRenderingContext::getVertexAttrib): Use cached value instead call glGetVertextAtrtrib.
Typo in glGetVertexAttrib. Also "instead of calling".


WebCore/html/canvas/WebGLRenderingContext.cpp:3741
 +      for (int ii = 0; ii < size; ++ii)
This loop needs to be from 0..expectedSize as well.

[Zhenyao Mo]

Created attachment 60319 [details]
revised patch

[Kenneth Russell]

Comment on attachment 60319 [details]
revised patch

Looks good. Thanks for your patience with the review.

[Dimitri Glazkov (Google)]

Comment on attachment 60319 [details]
revised patch

rs=me.

[WebKit Commit Bot]

Comment on attachment 60319 [details]
revised patch

Clearing flags on attachment: 60319

Committed r62385: <http://trac.webkit.org/changeset/62385>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.