Bug 51421 – Initialize to 0 for undefined values in CopyTexImage2D

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Zhenyao Mo]

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.

[Zhenyao Mo]

Created attachment 77365 [details]
Patch

[Zhenyao Mo]

Test is in sync with khronos.

[David Levin]

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".

[Kenneth Russell]

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?

[Zhenyao Mo]

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.

[Kenneth Russell]

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.

[Zhenyao Mo]

Created attachment 77558 [details]
Patch

[Zhenyao Mo]

(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.

[Kenneth Russell]

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)?

[Zhenyao Mo]

(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.

[Zhenyao Mo]

Created attachment 77565 [details]
Patch

[Kenneth Russell]

Comment on attachment 77565 [details]
Patch

Thanks for bearing with my questions. Looks very good. r=me

[Zhenyao Mo]

Committed r74720: <http://trac.webkit.org/changeset/74720>