Bug 41300 - Implement OpenGL ES 2.0 semantics for vertex attribute 0
Summary: Implement OpenGL ES 2.0 semantics for vertex attribute 0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks: 41572
  Show dependency treegraph
 
Reported: 2010-06-28 14:00 PDT by Kenneth Russell
Modified: 2010-07-03 15:03 PDT (History)
8 users (show)

See Also:


Attachments
patch (35.93 KB, patch)
2010-06-30 17:48 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: fix style issue (35.93 KB, patch)
2010-06-30 18:26 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: responding to gman's review (37.21 KB, patch)
2010-06-30 20:19 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: add a new test gl-vertex-attrib.html (39.83 KB, patch)
2010-07-01 15:59 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch : responding to Ken Russell's comments (40.31 KB, patch)
2010-07-01 16:49 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch (40.26 KB, patch)
2010-07-01 17:21 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2010-06-28 14:00:47 PDT
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.
Comment 1 Zhenyao Mo 2010-06-30 17:48:00 PDT
Created attachment 60175 [details]
patch
Comment 2 WebKit Review Bot 2010-06-30 18:19:10 PDT
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.
Comment 3 Zhenyao Mo 2010-06-30 18:26:18 PDT
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.
Comment 4 Zhenyao Mo 2010-06-30 19:37:51 PDT
Comments from Gregg Tavares <gman@google.com>

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.
Comment 5 Zhenyao Mo 2010-06-30 20:02:31 PDT
(In reply to comment #4)
> Comments from Gregg Tavares <gman@google.com>
> 
> 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.
Comment 6 Zhenyao Mo 2010-06-30 20:19:13 PDT
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.
Comment 7 Gregg Tavares 2010-06-30 23:43:20 PDT
LGTM
Comment 8 Zhenyao Mo 2010-07-01 15:59:55 PDT
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.
Comment 9 Kenneth Russell 2010-07-01 16:11:16 PDT
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.
Comment 10 Zhenyao Mo 2010-07-01 16:48:58 PDT
(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.
Comment 11 Zhenyao Mo 2010-07-01 16:49:36 PDT
Created attachment 60315 [details]
revised patch : responding to Ken Russell's comments
Comment 12 Kenneth Russell 2010-07-01 17:04:12 PDT
(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.
Comment 13 Zhenyao Mo 2010-07-01 17:21:33 PDT
Created attachment 60319 [details]
revised patch
Comment 14 Kenneth Russell 2010-07-01 17:29:18 PDT
Comment on attachment 60319 [details]
revised patch

Looks good. Thanks for your patience with the review.
Comment 15 Dimitri Glazkov (Google) 2010-07-02 08:45:01 PDT
Comment on attachment 60319 [details]
revised patch

rs=me.
Comment 16 WebKit Commit Bot 2010-07-02 09:35:14 PDT
Comment on attachment 60319 [details]
revised patch

Clearing flags on attachment: 60319

Committed r62385: <http://trac.webkit.org/changeset/62385>
Comment 17 WebKit Commit Bot 2010-07-02 09:35:20 PDT
All reviewed patches have been landed.  Closing bug.