[webkit-reviews] review denied: [Bug 110883] Add state variables to avoid OpenGL redundant state changes on GraphicsContext3D : [Attachment 190855] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Bruno Abinader
<bruno.abinader at basyskom.com>'s request for review:
Bug 110883: Add state variables to avoid OpenGL redundant state changes on
GraphicsContext3D
https://bugs.webkit.org/show_bug.cgi?id=110883

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

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


Sorry I didn't notice these before, but code inspection revealed a couple more
bugs. I hope you've tested this change thoroughly and that the performance
gains from these changes warrant the risk of caching these values.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1091
> +	       , sourceFactor(GraphicsContext3D::ZERO)

This value is wrong. The initial value is GraphicsContext3D::ONE.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1097
> +	       , scissorHeight(0)

The scissor width and height default to the initial width and height of the
drawable. I hope this cache will be correct enough.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1107
> +	       , viewportHeight(0)

Similar issue with the viewport width and height.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:204

> +	   pixelStorei(GL_PACK_ALIGNMENT, 4);

Sorry for not noticing this before -- but isn't this incorrect? You haven't
changed the call to ::glPixelStorei below in the same method, so after calling
this, the cached value in the pixelStoreIntMap will be incorrect. Since you're
querying the GL for the value here anyway, I think you should just leave this
alone.