[Webkit-unassigned] [Bug 102215] WebGL: Avoid unnecessary format conversion for texImage2D by using GL extension

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=102215


Kenneth Russell <kbr at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #183859|review?                     |review-
               Flag|                            |




--- Comment #4 from Kenneth Russell <kbr at google.com>  2013-01-22 20:22:59 PST ---
(From update of attachment 183859)
View in context: https://bugs.webkit.org/attachment.cgi?id=183859&action=review

This patch contains a major flaw: allowing JavaScript code to pass down the BGRA format into the WebGL implementation. It seems clear to me that inadequate testing was done on it, so the statement "Already covered by current tests" is insufficient. In addition to this and other issues, this patch would for the first time cause BGRA internalformat textures to be allocated via TexImage2D(..., HTMLImageElement, ...), which means that they would have to work seamlessly as FBO attachments, in copyTexSubImage2D calls, etc. With all of these considerations I am highly dubious that this optimization should be implemented at all, although I am open to further discussion and corrections to my analysis. r- because of the issues above.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:583
> +    m_isTextureFromatBGRA8888EXTSupported = m_context->getExtensions()->isEnabled("GL_EXT_texture_format_BGRA8888");

Typo: m_isTextureFromatBGRA8888EXTSupported -> m_isTextureFormatBGRA8888EXTSupported

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3907
> +    // FIXME: current chromium Implementation requires that the format parameter of texSubImage2D() should be the same with that of the previously called texImage2D() for the same texture which limits the usage of BGRA8 extension.

This restriction comes from OpenGL ES, not Chromium. The OpenGL ES 2.0.25 specification, section 3.7.2, states:

"The same constraints and errors apply to the TexSubImage commands' argument format and the internalformat of the texture array being respecified as apply to the format and internalformat arguments of its TexImage counterparts."

When WebKit is running on any port which uses OpenGL ES as its base graphics layer, rather than OpenGL, this restriction will be enforced. Chromium happens to enforce GLES semantics on all platforms for best portability. References to the Chromium port should be removed from these comments.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3916
> +    if (tex->getInternalFormat(target, level) == Extensions3D::BGRA_EXT) {

Throughout this patch, this new code is being added before the input validation which is done inside texSubImage2DBase. This makes the code very fragile. It looks to me like there are cases where this RGBA8->BGRA8 conversion code will be run where the later validation code in texSubImage2DBase will actually reject the call. Further, I can't convince myself that in illegal situations, this code won't try to swizzle input data that it doesn't really know the format of (in particular, in the overload taking ArrayBufferView*). On the whole, this patch looks like a bolt-on to the existing texture packing/unpacking code rather than being well integrated with it.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4917
> +    case Extensions3D::BGRA_EXT:

Adding this case statement here and in the other validation functions makes it possible for programs to pass down this constant from the public WebGL API, which is unacceptable. Did you run the WebGL conformance tests with this change? I am surprised they would pass. The conformance suite contains many negative tests to ensure that only enums in the spec can be passed across the API. If the conformance tests failed but the layout tests passed, then the failing conformance tests need to be imported as layout tests.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4985
> +    case Extensions3D::BGRA_EXT:

Same problem here.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.