Bug 47178 – [CHROMIUM] Layout Test fast/canvas/toDataURL-alpha.html failing with --accelerated-2d-canvas

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Stephen White]

run_webkit_tests --accelerated-2d-canvas fast/canvas/toDataURL-alpha.html

Expected two green squares.

Actual image contains artifacts in both squares.

[Stephen White]

See also http://crbug.com/57434.

[Stephen White]

Created attachment 69789 [details]
Patch

[Stephen White]

Comment on attachment 69789 [details]
Patch

Found a problem; removing r?.

[Stephen White]

Created attachment 70127 [details]
Patch

[Stephen White]

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

This patch pushes the responsibility for clearing out new textures down to WebGraphicsContext3DDefaultImpl.  This duplicates what the command buffer code was doing.  Since this behaviour is required by WebGL semantics (I think), and GraphicsContext3D is used to implement WebGL, all GraphicsContext3D implementations should do it.

Ideally, the command buffer clearing code should be refactored to a common place for both command buffers and WebKit, but I'm not sure what the correct place would be.

[James Robinson]

Comment on attachment 70127 [details]
Patch

How do we handle this in the command buffer case?  Seems really strange that we only see this in testing.

If the issue is just with the canvas 2d backbuffer then we should just do a glClear() after resizing with texImage2D.

[Stephen White]

(In reply to comment #6)
> (From update of attachment 70127 [details])
> How do we handle this in the command buffer case?  Seems really strange that we only see this in testing.

The command buffer equivalent to this code already clears out the pixels (see GLES2DecoderImpl::DoTexImage2D()).

> If the issue is just with the canvas 2d backbuffer then we should just do a glClear() after resizing with texImage2D.

I tried that in my first patch, but for some reason it messed up the Amazon Shelf demo.

I'm pretty sure this would also affect any WebGL or compositing/CSS3D tests that assume that textures are cleared to black, so we should preserve WebGL semantics for the in-process OSMesa implementation.

[Stephen White]

BTW, this change reduces the number of fails on canvas/philip/* from 39 to 3, so it'd be nice to get it in.

[James Robinson]

Comment on attachment 70127 [details]
Patch

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

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1241
> +// N.B.:  This code does not protect against integer overflow, so it should
> +// not be considered robust enough for production use.  Since the default impl
> +// is only used for testing, this should be ok for now.
> +int imageSizeInBytes(int width, int height, int format, int type)

Since this is calculating a value in bytes it should return a size_t.  Is the N.B. supposed to be a FIXME? How would it be fixed?

width/height etc are unsigned in the callers, they should be unsigned here as well.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1246
> +void WebGraphicsContext3DDefaultImpl::texImage2D(unsigned target, unsigned level, unsigned internalFormat, unsigned width, unsigned height, unsigned border, unsigned format, unsigned type, const void*pixels)

nit: void*pixels needs a space

[Stephen White]

(In reply to comment #9)
> (From update of attachment 70127 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70127&action=review
> 
> > WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1241
> > +// N.B.:  This code does not protect against integer overflow, so it should
> > +// not be considered robust enough for production use.  Since the default impl
> > +// is only used for testing, this should be ok for now.
> > +int imageSizeInBytes(int width, int height, int format, int type)
> 
> Since this is calculating a value in bytes it should return a size_t.

Fixed.

> Is the N.B. supposed to be a FIXME?

The comment means "this is ok since this implementation is only used for layout tests.  If you were crazy enough to use it for the browser (and you shouldn't, since it's inside the renderer process), it should be made more robust."  I cleaned up the wording a bit to reflect that.

> How would it be fixed?

It could be fixed the same way is is done on the command-buffer side:  all multiplies are done using a function which checks the args for overflow.  This seems like overkill for layout tests.

> width/height etc are unsigned in the callers, they should be unsigned here as well.
> 
> > WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1246
> > +void WebGraphicsContext3DDefaultImpl::texImage2D(unsigned target, unsigned level, unsigned internalFormat, unsigned width, unsigned height, unsigned border, unsigned format, unsigned type, const void*pixels)
> 
> nit: void*pixels needs a space

Fixed.

[Stephen White]

Created attachment 70531 [details]
Patch

[James Robinson]

Comment on attachment 70531 [details]
Patch

Makes sense

[Stephen White]

Committed r69588: <http://trac.webkit.org/changeset/69588>