run_webkit_tests --accelerated-2d-canvas fast/canvas/toDataURL-alpha.html Expected two green squares. Actual image contains artifacts in both squares.
See also http://crbug.com/57434.
Created attachment 69789 [details] Patch
Comment on attachment 69789 [details] Patch Found a problem; removing r?.
Created attachment 70127 [details] Patch
(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.
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.
(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.
BTW, this change reduces the number of fails on canvas/philip/* from 39 to 3, so it'd be nice to get it in.
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
(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.
Created attachment 70531 [details] Patch
Comment on attachment 70531 [details] Patch Makes sense
Committed r69588: <http://trac.webkit.org/changeset/69588>