Bug 188291 – WebGL 2 conformance: vertex_arrays/vertex_array_object.html

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Justin Fan]

WebGL 2 conformance: vertex_arrays/vertex_array_object.html

[Justin Fan]

<rdar://problem/42792709>

[Justin Fan]

Mac side is done, fixing iOS before uploading the patch.

[Justin Fan]

Created attachment 346439 [details]
Patch

[Justin Fan]

We shouldn’t need the _APPLE calls anymore; VAOs take the OES extension path for WebGL 1, whereas WebGL 2 is now backed by GL 4 or ES 3 which both natively support vertex arrays.

[Dean Jackson]

Comment on attachment 346439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346439&action=review

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:98
> +#if !USE(OPENGL_ES)

Swap this to be #if USE(OPENGL_ES)

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:99
> +    bindVertexArray(nullptr); // WebGL, unlike OpenGL 3.3+, expects a bound default VAO.

Could you expand on this comment a bit? It's also a bit confusing, because you're talking about WebGL but inside a block that is doing desktop OpenGL.

Maybe you could say something like "OpenGL 3.3+ removed the concept of default VAO. For that reason we need to explicitly bind to nothing before trying to initialise it."
Or something :)

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1070
> +#if !USE(OPENGL_ES)

Swap this to be #if USE(OPENGL_ES)

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1780
> +    if (buffer->isDeleted() || !deleteObject(buffer))

What causes this change? Where is the WebGLBuffer getting deleted?

> Source/WebCore/html/canvas/WebGLVertexArrayObject.cpp:55
> -    switch (m_type) {
> -    case Type::Default:
> -        break;
> -    case Type::User:
> -        setObject(this->context()->graphicsContext3D()->createVertexArray());
> -        break;
> -    }
> +#if !USE(OPENGL_ES)
> +    ASSERT(!(type == Type::Default && this->context()->m_defaultVertexArrayObject));
> +#else
> +    if (m_type != Type::User)
> +        return;
> +#endif
> +    setObject(this->context()->graphicsContext3D()->createVertexArray());

I suggest writing the #if with a positive conditional:

#if USE(OPENGL_ES)

I'd also write the ASSERT without the ! : type != Type::Default || !this->context()->m_defaultVertexArrayObject

[EWS Watchlist]

Comment on attachment 346439 [details]
Patch

Attachment 346439 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8744330

New failing tests:
webgl/2.0.0/conformance2/glsl3/array-assign.html
webgl/2.0.0/conformance2/glsl3/array-equality.html
webgl/2.0.0/conformance2/glsl3/tricky-loop-conditions.html
webgl/2.0.0/conformance2/glsl3/vector-dynamic-indexing.html
webgl/2.0.0/conformance2/glsl3/loops-with-side-effects.html
webgl/2.0.0/conformance2/glsl3/array-as-return-value.html
webgl/2.0.0/conformance2/glsl3/compare-structs-containing-arrays.html
webgl/2.0.0/conformance2/glsl3/no-attribute-vertex-shader.html
webgl/2.0.0/conformance2/glsl3/array-in-complex-expression.html
webgl/2.0.0/conformance2/glsl3/bool-type-cast-bug-uint-ivec-uvec.html
webgl/2.0.0/conformance2/glsl3/array-assign-constructor.html
webgl/2.0.0/conformance2/glsl3/array-element-increment.html
webgl/2.0.0/conformance2/glsl3/frag-depth.html
webgl/2.0.0/conformance2/glsl3/short-circuiting-in-loop-condition.html
webgl/2.0.0/conformance2/glsl3/vector-dynamic-indexing-nv-driver-bug.html
webgl/1.0.2/conformance/more/functions/deleteBufferBadArgs.html
webgl/2.0.0/conformance2/glsl3/const-array-init.html
webgl/2.0.0/conformance2/glsl3/array-complex-indexing.html

[EWS Watchlist]

Created attachment 346442 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Justin Fan]

(In reply to Dean Jackson from comment #5)
> Comment on attachment 346439 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346439&action=review

Got it, thanks!

> > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1780
> > +    if (buffer->isDeleted() || !deleteObject(buffer))
> 
> What causes this change? Where is the WebGLBuffer getting deleted?

For this, WebGL 1 spec states that most redundant delete functions (e.g. binding two VAOs to a single buffer, then binding the first VAO and calling deleteBuffer; binding the second VAO and calling deleteBuffer should do nothing) should be no-ops. Since we weren't preventing this, the conformance test was failing. I actually have a more general change that should affect deleteProgram, deleteShader, deleteTexture, deleteRenderBuffer, and deleteFrameBuffer as well as per the spec. 

Going to throw another patch at EWS, this time with the new test passes in /glsl3 and fixing the crash in deleteBufferBadArgs.

[EWS Watchlist]

Comment on attachment 346439 [details]
Patch

Attachment 346439 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8744502

New failing tests:
webgl/2.0.0/conformance2/glsl3/array-assign.html
webgl/2.0.0/conformance2/glsl3/array-equality.html
webgl/2.0.0/conformance2/glsl3/tricky-loop-conditions.html
webgl/2.0.0/conformance2/glsl3/vector-dynamic-indexing.html
webgl/2.0.0/conformance2/glsl3/loops-with-side-effects.html
webgl/2.0.0/conformance2/glsl3/array-as-return-value.html
webgl/2.0.0/conformance2/glsl3/compare-structs-containing-arrays.html
webgl/2.0.0/conformance2/glsl3/no-attribute-vertex-shader.html
webgl/2.0.0/conformance2/glsl3/array-in-complex-expression.html
webgl/2.0.0/conformance2/glsl3/bool-type-cast-bug-uint-ivec-uvec.html
webgl/2.0.0/conformance2/glsl3/array-assign-constructor.html
webgl/2.0.0/conformance2/glsl3/array-element-increment.html
webgl/2.0.0/conformance2/glsl3/frag-depth.html
webgl/2.0.0/conformance2/glsl3/short-circuiting-in-loop-condition.html
webgl/2.0.0/conformance2/glsl3/vector-dynamic-indexing-nv-driver-bug.html
webgl/1.0.2/conformance/more/functions/deleteBufferBadArgs.html
webgl/2.0.0/conformance2/glsl3/const-array-init.html
webgl/2.0.0/conformance2/glsl3/array-complex-indexing.html

[EWS Watchlist]

Created attachment 346443 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Justin Fan]

Created attachment 346445 [details]
Patch

[Justin Fan]

Created attachment 346450 [details]
Patch

[Justin Fan]

Created attachment 346456 [details]
Patch

[EWS Watchlist]

Comment on attachment 346456 [details]
Patch

Attachment 346456 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8750945

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html

[EWS Watchlist]

Created attachment 346491 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[WebKit Commit Bot]

Comment on attachment 346456 [details]
Patch

Rejecting attachment 346456 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 346456, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/8753272

[Justin Fan]

Created attachment 346553 [details]
Patch with reviewer added

[Justin Fan]

Created attachment 346555 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 346555 [details]
Patch

Clearing flags on attachment: 346555

Committed r234566: <https://trac.webkit.org/changeset/234566>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.