Bug 47178 - [CHROMIUM] Layout Test fast/canvas/toDataURL-alpha.html failing with --accelerated-2d-canvas
Summary: [CHROMIUM] Layout Test fast/canvas/toDataURL-alpha.html failing with --accele...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-05 07:52 PDT by Stephen White
Modified: 2010-10-12 10:31 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.48 KB, patch)
2010-10-05 08:30 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (3.80 KB, patch)
2010-10-07 12:06 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (3.91 KB, patch)
2010-10-12 08:38 PDT, Stephen White
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2010-10-05 07:52:39 PDT
run_webkit_tests --accelerated-2d-canvas fast/canvas/toDataURL-alpha.html

Expected two green squares.

Actual image contains artifacts in both squares.
Comment 1 Stephen White 2010-10-05 07:53:17 PDT
See also http://crbug.com/57434.
Comment 2 Stephen White 2010-10-05 08:30:56 PDT
Created attachment 69789 [details]
Patch
Comment 3 Stephen White 2010-10-05 08:37:33 PDT
Comment on attachment 69789 [details]
Patch

Found a problem; removing r?.
Comment 4 Stephen White 2010-10-07 12:06:03 PDT
Created attachment 70127 [details]
Patch
Comment 5 Stephen White 2010-10-07 12:12:02 PDT
(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 6 James Robinson 2010-10-07 16:50:15 PDT
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.
Comment 7 Stephen White 2010-10-07 19:34:57 PDT
(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.
Comment 8 Stephen White 2010-10-08 13:11:14 PDT
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 9 James Robinson 2010-10-08 14:03:47 PDT
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
Comment 10 Stephen White 2010-10-12 08:38:12 PDT
(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.
Comment 11 Stephen White 2010-10-12 08:38:43 PDT
Created attachment 70531 [details]
Patch
Comment 12 James Robinson 2010-10-12 09:30:07 PDT
Comment on attachment 70531 [details]
Patch

Makes sense
Comment 13 Stephen White 2010-10-12 10:31:27 PDT
Committed r69588: <http://trac.webkit.org/changeset/69588>