[webkit-reviews] review denied: [Bug 102215] WebGL: Avoid unnecessary format conversion for texImage2D by using GL extension : [Attachment 183859] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Jun Jiang <jun.a.jiang at intel.com>'s
request for review:
Bug 102215: WebGL: Avoid unnecessary format conversion for texImage2D by using
GL extension
https://bugs.webkit.org/show_bug.cgi?id=102215

Attachment 183859: Patch
https://bugs.webkit.org/attachment.cgi?id=183859&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
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.