[webkit-reviews] review denied: [Bug 102161] WebGL: Avoid unnecessary memory copy or conversion in texImage2D and texSubImage2D for HTMLVideoElement : [Attachment 177448] 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 102161: WebGL: Avoid unnecessary memory copy or conversion in texImage2D
and texSubImage2D for HTMLVideoElement
https://bugs.webkit.org/show_bug.cgi?id=102161

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

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


Nice results but the code needs more work.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3634
> +	       || (alphaOp == GraphicsContext3D::AlphaDoPremultiply &&
alphaValue == 0xFF))

It isn't legal to make this determination based on the alpha value of the
upper-left pixel of the video. Please make the code correct in all cases.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3781
> +    // if the graphics port provides support to the DontCopyBackingStore
semantics. SKIA, CG, CAIRO and QT ports already support this
DontCopyBackingStore behavior. 

Where is it specified that DontCopyBackingStore is supported by these ports?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3782
> +#if USE(SKIA) || USE(CG) || USE(CAIRO) || PLATFORM(QT)

How is this #if going to be kept up-to-date as more ports begin to support the
optimization? An API which each port implements is needed rather than an #ifdef
here.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3894
> +	       alphaOp = GraphicsContext3D::AlphaDoNothing;

Same comments as above.