[webkit-reviews] review denied: [Bug 110818] [WebGL] Support for texImage2D of type HALF_FLOAT_OES with null pixels. : [Attachment 190335] oes_texture_half_float

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Nayan Kumar K
<nayankk at motorola.com>'s request for review:
Bug 110818: [WebGL] Support for texImage2D of type HALF_FLOAT_OES with null
pixels.
https://bugs.webkit.org/show_bug.cgi?id=110818

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=190335&action=review


Thanks for continuing to push this patch forward. Before it is committed I
think you should get it running in the Mac port of DRT. You probably need to
check for the GL_ARB_half_float_pixel OpenGL extension, and if it's present,
then have GraphicsContext3D claim that it supports GL_OES_texture_half_float.
Also, the patch needs to be rebaselined and pass the EWS bots.

> Source/WebCore/html/canvas/OESTextureHalfFloat.cpp:2
> + * Copyright (C) 2013 Google Inc. All rights reserved.

You might want to change the attribution to Motorola.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:5140
> +	   break;

In addition to these checks, please add validation to the texImage2D and
texSubImage2D entry points making sure that you can't upload ImageData,
HTMLImageElement, HTMLCanvasElement, or HTMLVideoElement as HALF_FLOAT textures
yet. Also, add tests verifying this.

> LayoutTests/fast/canvas/webgl/oes-texture-half-float-expected.txt:1
> +CONSOLE MESSAGE: WebGL: INVALID_ENUM: texImage2D: invalid texture type

Is this console message coming from the Chromium port of DRT? If so, you should
not check in this result into fast/canvas/webgl/ , but into platform/chromium/
instead.

> LayoutTests/fast/canvas/webgl/oes-texture-half-float.html:16
> +<!-- Shaders for testing half-floating-point textures -->

If you aren't going to add tests verifying rendering to HALF_FLOAT_OES textures
yet, then remove these unused shaders and associated code for now.

> LayoutTests/fast/canvas/webgl/oes-texture-half-float.html:131
> +    gl.texImage2D(gl.TEXTURE_2D, 0, format, width, height, 0, format,
gl.HALF_FLOAT_OES, data);

You should also verify that allocation with null data succeeds.

> LayoutTests/fast/canvas/webgl/oes-texture-half-float.html:138
> +    }

Please either add a test verifying that you can render to a HALF_FLOAT_OES
texture, similar to the oes-texture-float test, or add a FIXME indicating that
such a test is needed.

> LayoutTests/fast/canvas/webgl/oes-texture-half-float.html:139
> +}

Please add missing negative tests ensuring that you can't upload ImageData,
HTMLImageElement, HTMLCanvasElement, or HTMLVideoElement into HALF_FLOAT_OES
textures yet.