Bug 49355 – WebGLRenderingContext needs to zero textures and renderbuffers

• Component: WebGL
• Status: RESOLVED FIXED

[Chris Marrin]

WebGL must not have uninitialized data in any buffer. Currently if you pass 0  for the pixel pointer, the texture will be uninitialized.

[Zhenyao Mo]

I think should do it in WebGLRenderingContext::texImage2DBase(), because 1) this will take care of all ports, 2) framebuffer initialization also happens at WebGLRenderingContext level.

If you don't mind, I can take this bug as we need to remove the duplicated code in Chromium.

[Kenneth Russell]

Agreed -- Chris and I discussed this over IRC and we agree that the texture and renderbuffer initialization should be done at levels above GraphicsContext3D, but only if the GC3D implementation doesn't already do it (need to add another method like clearsTexturesAndRenderbuffers()).

Reassigning and changing synopsis.

[Zhenyao Mo]

Created attachment 74056 [details]
patch

Test is in sync with khronos.

[Eric Seidel (no email)]

Attachment 74056 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6153001

[Zhenyao Mo]

Created attachment 74058 [details]
revised patch: fix compile issue for webkit

[Kenneth Russell]

Comment on attachment 74058 [details]
revised patch: fix compile issue for webkit

View in context: https://bugs.webkit.org/attachment.cgi?id=74058&action=review

This mostly looks good. Minor code and formatting issues.

> WebCore/html/canvas/WebGLRenderingContext.cpp:2512
> +        zero.set(new unsigned char[size]);

This should be "zero = adoptArrayPtr(new unsigned char[size]);".

> LayoutTests/fast/canvas/webgl/uninitialized-test.html:75
> +if (data.length != expectedDataLength) {
> +    testFailed("expected data length " + expectedDataLength + " but got " + data.length + " instead.");
> +} else {
> +    var k = 0;
> +    for (var i = 0; i < data.length; ++i) {
> +	if (data[i] != 0) {
> +	    k++;
> +	}
> +    }
> +
> +    if (k) {
> +	testFailed("Found " + k + " non-zero bytes");
> +    } else {
> +	testPassed("All data initialized");
> +    }
> +}

Indentation is very messed up.

[Zhenyao Mo]

Created attachment 74140 [details]
revised patch: responding to kbr's review

Fixed the test indent issue on khronos and synced with it.

[Kenneth Russell]

Comment on attachment 74140 [details]
revised patch: responding to kbr's review

Looks good.

[Zhenyao Mo]

Committed r72228: <http://trac.webkit.org/changeset/72228>

[David Levin]

Rolled out here: http://trac.webkit.org/changeset/72273

It caused many >10 regressions on Win and Linux gpu layout test runs.

[David Levin]

Comment on attachment 74140 [details]
revised patch: responding to kbr's review

Clearing r+ and obsoleting bad patch.

After this was rolled out, the gpu layout tests started passing.

[Zhenyao Mo]

Comment on attachment 74140 [details]
revised patch: responding to kbr's review

This patch is actually correct, only that it exposes a bug in DrawingBuffer.cpp:reset().  Will wait until the fix for that lands to ask review again.

[David Levin]

(In reply to comment #12)
> (From update of attachment 74140 [details])
> This patch is actually correct, only that it exposes a bug in DrawingBuffer.cpp:reset().  Will wait until the fix for that lands to ask review again.

Regardless of correctness, the key idea is to run the gpu layout tests (and address failures) OR else you leave the gardener with some odd choices:

1. Try to rebaseline things. (I'm not sure if we have instructions about how to do this for the gpu tests. Also we have to look hard at the results and try to determine if they are correct or there is a bug, etc.)

2. Roll it out. Also, not a trivial decision.

Multiply this by lots of patches and then the gardener is having to potentially work 15+ hours days to keep up when tests aren't run :(

[Zhenyao Mo]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 74140 [details] [details])
> > This patch is actually correct, only that it exposes a bug in DrawingBuffer.cpp:reset().  Will wait until the fix for that lands to ask review again.
> 
> Regardless of correctness, the key idea is to run the gpu layout tests (and address failures) OR else you leave the gardener with some odd choices:
> 
> 1. Try to rebaseline things. (I'm not sure if we have instructions about how to do this for the gpu tests. Also we have to look hard at the results and try to determine if they are correct or there is a bug, etc.)
> 
> 2. Roll it out. Also, not a trivial decision.
> 
> Multiply this by lots of patches and then the gardener is having to potentially work 15+ hours days to keep up when tests aren't run :(

I am sorry to put you through this.  I didn't realize my change will effect something other than WebGL.  Otherwise I will definitely run the gpu tests before committing.

[Zhenyao Mo]

Created attachment 75761 [details]
revised patch

Removing all the cases where pixels==null is passed to GraphicsContext3D::texImagde2D().

Per discussion with kbr, now an INVALID_VALUE is generated in GraphicsContext3D::texImagde2D if pixels==null.

Tested in linux with --platform chromium-gpu and it's no longer crashing.

[Kenneth Russell]

Comment on attachment 75761 [details]
revised patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75761&action=review

> WebCore/html/canvas/WebGLRenderingContext.cpp:2534
> +        zero = adoptArrayPtr(new unsigned char[size]);
> +        memset(zero.get(), 0, size);

This construct is repeated many times throughout this code. It would be cleaner to use OwnFastMallocPtr<T> in conjunction with fastZeroedMalloc. The only issue is that OwnFastMallocPtr doesn't have a default constructor or assignment operator. Please go ahead and add them, and use them instead. (I don't think it's necessary to add a PassOwnFastMallocPtr class, just an assignment operator taking T*.)

[Zhenyao Mo]

Created attachment 76106 [details]
revised patch

Per discussion offline with kbr, a helper function texImage2DResourceSafe is added to GraphicsContext3D and used in various places in this patch.

[Zhenyao Mo]

Created attachment 76112 [details]
revised patch: fix a tiny issue

[Zhenyao Mo]

Created attachment 76130 [details]
Patch

[Kenneth Russell]

Comment on attachment 76130 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76130&action=review

The refactoring looks good but please clean up this one issue.

> WebCore/platform/graphics/GraphicsContext3D.h:477
> +    int texImage2DResourceSafe(unsigned target, unsigned level, unsigned internalformat, unsigned width, unsigned height, unsigned border, unsigned format, unsigned type);

Returning int is gross. Please make this return bool.

[Zhenyao Mo]

Created attachment 76148 [details]
Patch

[Kenneth Russell]

Comment on attachment 76148 [details]
Patch

Looks good. Thanks for persisting with this fix.

[Zhenyao Mo]

I think it landed, not sure why this hasn't been updated.

Committed r74440: <http://trac.webkit.org/changeset/74440>

The patch was tested on linux with --platform chromium-gpu, but not on Mac as my Mac is snowleopard.

Will watch the bots.

[Eric Seidel (no email)]

Landed by you or by the commit-queue?  (Wondering if I need to go look at the commit-queue logs for failures.)

[Zhenyao Mo]

Landed by me using webkit-patch land --ignore-builders

[Eric Seidel (no email)]

Hmm... If you saw an error message from webkit-patch land, I'd love to see it. :)

Also, I don't think --ignore-builders even exists anymore.  We certainly don't block lands on them. :)

[Zhenyao Mo]

(In reply to comment #26)
> Hmm... If you saw an error message from webkit-patch land, I'd love to see it. :)
> 
> Also, I don't think --ignore-builders even exists anymore.  We certainly don't block lands on them. :)

Unfortunately I didn't see any error messages.