Teach WebGL code about new buffer targets in WebGL2
Created attachment 292680 [details] WIP
Created attachment 292839 [details] Patch
Comment on attachment 292839 [details] Patch Attachment 292839 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2370827 New failing tests: fast/canvas/webgl/bufferData-offset-length.html svg/wicd/test-rightsizing-b.xhtml
Created attachment 292845 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 292839 [details] Patch Attachment 292839 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2370895 New failing tests: fast/canvas/webgl/bufferData-offset-length.html
Created attachment 292851 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 292839 [details] Patch Attachment 292839 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2371058 New failing tests: fast/canvas/webgl/bufferData-offset-length.html
Created attachment 292854 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 292879 [details] Patch
Created attachment 292880 [details] Patch
Comment on attachment 292880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292880&action=review > Source/WebCore/html/canvas/WebGLBuffer.cpp:158 > + m_byteLength = byteLength; This is wrong.
Comment on attachment 292880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292880&action=review > Source/WebCore/ChangeLog:16 > + In order to test this correctly, I had to upgrade the macOS's OpenGL > + context to a 3.2-compatible context to make sure the new binding > + points don't cause errors. Eventually, this 3.2-compatible context > + will have to be reverted and replaced with an ANGLE context. The current > + 3.2-compatible context is just for testing. As I say below, this is great. But I think it should land in a separate patch. You can consider this an r+ for it. > Source/WebCore/html/canvas/WebGLBuffer.cpp:239 > + switch (target) { > + case GraphicsContext3D::ARRAY_BUFFER: > + case GraphicsContext3D::ELEMENT_ARRAY_BUFFER: > m_target = target; > + break; > + default: > +#if ENABLE(WEBGL2) > + if (forWebGL2) { > + switch (target) { > + case GraphicsContext3D::COPY_READ_BUFFER: > + case GraphicsContext3D::COPY_WRITE_BUFFER: > + case GraphicsContext3D::PIXEL_PACK_BUFFER: > + case GraphicsContext3D::PIXEL_UNPACK_BUFFER: > + case GraphicsContext3D::TRANSFORM_FEEDBACK_BUFFER: > + case GraphicsContext3D::UNIFORM_BUFFER: > + m_target = target; > + break; > + } > + } > +#else > + UNUSED_PARAM(forWebGL2); > +#endif I don't like this switch within a switch, with the same test. It might have been better as it was originally, with the if statement for the two cases we know should set the target all the time. Then the test for WebGL2, with the switch child. Also, it's a shame we have to do both #if WEBGL2 and if (forWebGL2). I guess the only way to avoid that would be to define all the GC3D things like UNIFORM_BUFFER in all cases. Actually, maybe that's ok. GC3D should be a GLES3-like API, with WebGLRenderingContext being the thing that stops that from exposure to the web. What do you think? > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:928 > - if (target == GraphicsContext3D::ARRAY_BUFFER) > + switch (target) { > + case GraphicsContext3D::ARRAY_BUFFER: > m_boundArrayBuffer = buffer; > - else if (target == GraphicsContext3D::ELEMENT_ARRAY_BUFFER) > + break; > + case GraphicsContext3D::ELEMENT_ARRAY_BUFFER: > m_boundVertexArrayObject->setElementArrayBuffer(buffer); > - else { > + break; > + default: > +#if ENABLE(WEBGL2) > + bool success = true; > + if (isWebGL2()) { > + switch (target) { Same comment for this one. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:2206 > + bool valid = false; > + switch (target) { > + case GraphicsContext3D::ARRAY_BUFFER: > + case GraphicsContext3D::ELEMENT_ARRAY_BUFFER: > + valid = true; And here. > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:129 > + if (useGLES3) { > + // FIXME: Instead of backing a WebGL2 GraphicsContext3D with a OpenGL 3.2 context, we should instead back it with ANGLE. > + // Use an OpenGL 3.2 context for now until the ANGLE backend is ready. > + attribs.append(kCGLPFAOpenGLProfile); > + attribs.append(static_cast<CGLPixelFormatAttribute>(kCGLOGLPVersion_3_2_Core)); > + } This is good. But could you land it separately? > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:278 > - ::glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); > - ::glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); > - ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP); > - ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP); > + ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); > + ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); > + ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); > + ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); Nice! > Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:228 > +#if PLATFORM(MAC) > + if (m_useIndexedGetString) { > + GLint numExtensions = 0; > + ::glGetIntegerv(GL_NUM_EXTENSIONS, &numExtensions); > + for (GLint i = 0; i < numExtensions; ++i) > + m_availableExtensions.add(glGetStringi(GL_EXTENSIONS, i)); > + } else > +#endif > + { > + String extensionsString = getExtensions(); > + Vector<String> availableExtensions; > + extensionsString.split(' ', availableExtensions); > + for (size_t i = 0; i < availableExtensions.size(); ++i) > + m_availableExtensions.add(availableExtensions[i]); > + } Why didn't you change getExtensions() instead of putting this here? Also, you should explain this in the ChangeLog. > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:301 > + if (isGLES2Compliant()) { > + ASSERT(::glGetError() == GL_NO_ERROR); > + ::glGetIntegerv(GL_MAX_VARYING_VECTORS, value); > + if (::glGetError() == GL_INVALID_ENUM) { > + ::glGetIntegerv(GL_MAX_VARYING_COMPONENTS, value); > + *value /= 4; > + } > + } else { > + ::glGetIntegerv(GL_MAX_VARYING_FLOATS, value); > + *value /= 4; > + } Explain this in the ChangeLog.
Comment on attachment 292880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292880&action=review >> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:228 >> + } > > Why didn't you change getExtensions() instead of putting this here? > > Also, you should explain this in the ChangeLog. getExtensions() returns a string. It seems silly to build up a string just to immediately split it up again.
Comment on attachment 292880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292880&action=review >> Source/WebCore/ChangeLog:16 >> + 3.2-compatible context is just for testing. > > As I say below, this is great. But I think it should land in a separate patch. You can consider this an r+ for it. https://bugs.webkit.org/show_bug.cgi?id=164091
Created attachment 293101 [details] Patch
Created attachment 293102 [details] Patch for committing
Comment on attachment 293102 [details] Patch for committing Clearing flags on attachment: 293102 Committed r208032: <http://trac.webkit.org/changeset/208032>
All reviewed patches have been landed. Closing bug.