For example, if texture is larger than the currently bound fbo, then the out-of-bounds regions are undefined. Need to initialize them to 0 to avoid potential information leaking. Note that if the underlying GL implementation is already resource-safe (like chromium command buffer port), then no need to do it twice.
Created attachment 77365 [details] Patch
Test is in sync with khronos.
Comment on attachment 77365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77365&action=review This change would benefit from someone more familiar with the ara looking it over but here are a few minor nits I noticed when I looked briefly. > WebCore/html/canvas/WebGLFramebuffer.cpp:113 > +void WebGLFramebuffer::setAttachment(unsigned long attachment, WebGLRenderbuffer* rbo) Don't use abbreviations "rbo". > WebCore/html/canvas/WebGLFramebuffer.cpp:208 > + } else if (m_colorAttachment->isTexture()) { No braces for this single line if statement. > WebCore/html/canvas/WebGLFramebuffer.h:45 > + void setAttachment(unsigned long attachment, unsigned long texTarget, WebGLTexture* texture, int level); In function declarations, don't include variables that don't ad information. (Remove "texture".) > WebCore/html/canvas/WebGLFramebuffer.h:46 > + void setAttachment(unsigned long attachment, WebGLRenderbuffer* rbo); Don't include "rbo".
Comment on attachment 77365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77365&action=review This mostly looks good. Aside from Dave's nits, a couple of requests / questions. > WebCore/html/canvas/WebGLRenderingContext.cpp:81 > + if (start < 0) { > + range += start; > + start = 0; > + } Is this really the right behavior? Shouldn't the range be reduced by the amount that start is less than zero, regardless of whether end is within sourceRange? > WebCore/platform/graphics/GraphicsContext3D.h:800 > + IntSize getInternalBufferSize(); Please rename this to "getInternalFrameBufferSize". Then the comment (which as written doesn't add much) is really unnecessary. > LayoutTests/fast/canvas/webgl/uninitialized-test.html:128 > +gl.finish(); How about another test which passes down a negative x and y?
Comment on attachment 77365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77365&action=review >> WebCore/html/canvas/WebGLRenderingContext.cpp:81 >> + } > > Is this really the right behavior? Shouldn't the range be reduced by the amount that start is less than zero, regardless of whether end is within sourceRange? I think the code did exactly what you said.
Comment on attachment 77365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77365&action=review Would you be willing to upload a revised patch addressing the other issues? >>> WebCore/html/canvas/WebGLRenderingContext.cpp:81 >>> + } >> >> Is this really the right behavior? Shouldn't the range be reduced by the amount that start is less than zero, regardless of whether end is within sourceRange? > > I think the code did exactly what you said. Ah, I see now; sorry about that.
Created attachment 77558 [details] Patch
(In reply to comment #7) > Created an attachment (id=77558) [details] > Patch This patch addressed the comments from levin and kbr. Will sync the added test section (negative x/y) to khronos once it's reviewed here.
Comment on attachment 77558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77558&action=review > LayoutTests/fast/canvas/webgl/uninitialized-test.html:134 > +var x = -16; > +var y = -16; Would it be better to choose different values than 16 here? Will the fact that these are the same magnitude as fboWidth/fboHeight potentially hide bugs? > LayoutTests/fast/canvas/webgl/uninitialized-test.html:136 > +checkNonZeroPixels(tex, width, height, -x, -y, fboWidth, fboHeight, 255, 0, 0, 255); Are the x and y coordinates being passed to checkNonZeroPixels correct here? Should they be (0, 0)?
(In reply to comment #9) > (From update of attachment 77558 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77558&action=review > > > LayoutTests/fast/canvas/webgl/uninitialized-test.html:134 > > +var x = -16; > > +var y = -16; > > Would it be better to choose different values than 16 here? Will the fact that these are the same magnitude as fboWidth/fboHeight potentially hide bugs? Will revise. > > > LayoutTests/fast/canvas/webgl/uninitialized-test.html:136 > > +checkNonZeroPixels(tex, width, height, -x, -y, fboWidth, fboHeight, 255, 0, 0, 255); > > Are the x and y coordinates being passed to checkNonZeroPixels correct here? Should they be (0, 0)? I think -x and -y are the correct value here. Because starting point is (x, y), so the offset is (-x, -y) in the pixel buffer.
Created attachment 77565 [details] Patch
Comment on attachment 77565 [details] Patch Thanks for bearing with my questions. Looks very good. r=me
Committed r74720: <http://trac.webkit.org/changeset/74720>