if the region is beyond the currently bound fbo's bounds, we need to either clean undefined pixels to 0 or make sure they stay unchanged. Undefined leads to potential leaking.
After discussing it on public_webgl email list, we are going to set the undefined pixels to 0.
Created attachment 78134 [details] Patch
(In reply to comment #2) > Created an attachment (id=78134) [details] > Patch Will commit the test changes to khronos once reviewed.
Attachment 78134 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7374010
Comment on attachment 78134 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78134&action=review > WebCore/html/canvas/WebGLRenderingContext.cpp:725 > + long clippedX, clippedY, clippedWidth, clippedHeight; > + clip(x, width, getBoundFramebufferWidth(), &clippedX, &clippedWidth); > + clip(y, height, getBoundFramebufferHeight(), &clippedY, &clippedHeight); > + if (clippedX != x || clippedY != y || clippedWidth != width || clippedHeight != height) { > + unsigned long format = tex->getInternalFormat(target, level); Shouldn't this huge block be a new helper function? It could (should!) even be just static to the file.
Attachment 78134 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7401013
(In reply to comment #5) > (From update of attachment 78134 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78134&action=review > > > WebCore/html/canvas/WebGLRenderingContext.cpp:725 > > + long clippedX, clippedY, clippedWidth, clippedHeight; > > + clip(x, width, getBoundFramebufferWidth(), &clippedX, &clippedWidth); > > + clip(y, height, getBoundFramebufferHeight(), &clippedY, &clippedHeight); > > + if (clippedX != x || clippedY != y || clippedWidth != width || clippedHeight != height) { > > + unsigned long format = tex->getInternalFormat(target, level); > > Shouldn't this huge block be a new helper function? It could (should!) even be just static to the file. I think it's fine in its current form. It isn't duplicated code, and accesses members several times (m_context, getBoundFramebufferWidth/Height) making it inconvenient to write as anything but a method.
Created attachment 78166 [details] Patch
(In reply to comment #8) > Created an attachment (id=78166) [details] > Patch This patch fixes the build failure on chromium. Also, I refactor the code a little bit, adding a new function clip2D and call it in both copyTexImage2D and copyTexSubImage2D.
Comment on attachment 78166 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78166&action=review This basically looks good. One issue needing a revision. > WebCore/html/canvas/WebGLRenderingContext.cpp:752 > + m_context->texSubImage2D(target, level, xoffset, yoffset, width, height, format, type, zero.get()); The unpack alignment needs to be set to 1 before this call and restored afterward. See the various WebGLRenderingContext::texSubImage2D variants in this file.
Created attachment 78243 [details] Patch
Comment on attachment 78243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78243&action=review Looks good - one minor issue with the new code which can be fixed upon landing. > WebCore/html/canvas/WebGLRenderingContext.cpp:752 > + if (zero.get()) This can just be if (zero). These pointer types have an operator which converts to boolean type. > WebCore/html/canvas/WebGLRenderingContext.cpp:755 > + if (zero.get()) Here too.
Committed r75252: <http://trac.webkit.org/changeset/75252>
This may have caused a build fail on the canary: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/5595/steps/compile/logs/stdio
(In reply to comment #14) > This may have caused a build fail on the canary: > > http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/5595/steps/compile/logs/stdio I am on it. Just a sec.
This should fix it: r75254: <http://trac.webkit.org/changeset/75254>
(In reply to comment #16) > This should fix it: r75254: <http://trac.webkit.org/changeset/75254> Thanks!