Bug 51559 – copyTexSubImage2D shouldn't have undefined pixels

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

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.

[Zhenyao Mo]

After discussing it on public_webgl email list, we are going to set the undefined pixels to 0.

[Zhenyao Mo]

Created attachment 78134 [details]
Patch

[Zhenyao Mo]

(In reply to comment #2)
> Created an attachment (id=78134) [details]
> Patch

Will commit the test changes to khronos once reviewed.

[WebKit Review Bot]

Attachment 78134 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7374010

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

Attachment 78134 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7401013

[Kenneth Russell]

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

[Zhenyao Mo]

Created attachment 78166 [details]
Patch

[Zhenyao Mo]

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

[Kenneth Russell]

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.

[Zhenyao Mo]

Created attachment 78243 [details]
Patch

[Kenneth Russell]

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.

[Zhenyao Mo]

Committed r75252: <http://trac.webkit.org/changeset/75252>

[Stephen White]

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

[Zhenyao Mo]

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

[Zhenyao Mo]

This should fix it: r75254: <http://trac.webkit.org/changeset/75254>

[Stephen White]

(In reply to comment #16)
> This should fix it: r75254: <http://trac.webkit.org/changeset/75254>

Thanks!