[webkit-reviews] review granted: [Bug 197900] Please support WEBGL_compressed_texture_etc1 extension (and possibly WEBGL_compressed_texture_etc too) : [Attachment 382872] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Kenneth Russell <kbr at google.com>'s
request for review:
Bug 197900: Please support WEBGL_compressed_texture_etc1 extension (and
possibly WEBGL_compressed_texture_etc too)
https://bugs.webkit.org/show_bug.cgi?id=197900

Attachment 382872: Patch

https://bugs.webkit.org/attachment.cgi?id=382872&action=review




--- Comment #15 from Dean Jackson <dino at apple.com> ---
Comment on attachment 382872
  --> https://bugs.webkit.org/attachment.cgi?id=382872
Patch

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

Thanks for turning on ASTC as well. We hadn't done that yet because we were
worried about moving to a GLES 3 backend.

> Source/WebCore/html/canvas/WebGLCompressedTextureETC.cpp:64
> +    bool res =
context.graphicsContext3D()->getExtensions().supports("GL_ANGLE_compressed_text
ure_etc");
> +    os_log(OS_LOG_DEFAULT, "WebGLCompressedTextureETC::supported returning
%d", (int) res);
> +    return res;

I think you accidentally left this logging in.

> Source/WebCore/html/canvas/WebGLCompressedTextureETC1.cpp:55
> +    bool res =
context.graphicsContext3D()->getExtensions().supports("GL_OES_compressed_ETC1_R
GB8_texture");
> +    os_log(OS_LOG_DEFAULT, "WebGLCompressedTextureETC1::supported returning
%d", (int) res);
> +    return res;

Same here.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5575
> +    case Extensions3D::ETC1_RGB8_OES:
>	   {

Nit: I believe our style guide says the { here should go on the line above -
although it might not be consistently applied in this file.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5653
> +	   {

Ditto.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5656
> +	       bytesRequired = (width + kEACAndETC2BlockSize - 1) /
kEACAndETC2BlockSize;
> +	       bytesRequired *= (height + kEACAndETC2BlockSize - 1) /
kEACAndETC2BlockSize;
> +	       bytesRequired *= 8;

Could this ever overflow?

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5663
> +	   {

Ditto.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5666
> +	       bytesRequired = (width + kEACAndETC2BlockSize - 1) /
kEACAndETC2BlockSize;
> +	       bytesRequired *= (height + kEACAndETC2BlockSize - 1) /
kEACAndETC2BlockSize;
> +	       bytesRequired *= 16;

Overflow?