Bug 66036

Summary: Canvas fill and fillRect with SourceIn, DestinationIn, SourceOut, DestinationAtop and Copy have errors
Product: WebKit Reporter: Ben Wells <benwells>
Component: New BugsAssignee: Ben Wells <benwells>
Status: RESOLVED FIXED    
Severity: Normal CC: adamk, benwells, dglazkov, jamesr, mikelawther, senorblanco, stefanwehrmeyer, tomhudson, twiz, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 65583    
Attachments:
Description Flags
Patch
none
Test case showing fill and fillRect in all modes with transforms and clips.
none
Patch
none
Patch updated for comments
none
Patch
none
Patch
none
Webpage capture showing errors none

Description Ben Wells 2011-08-10 23:53:32 PDT
Canvas compositing modes SourceIn, DestinationIn, SourceOut, DestinationAtop and Copy have errors
Comment 1 Ben Wells 2011-08-10 23:56:39 PDT
Created attachment 103585 [details]
Patch
Comment 2 Ben Wells 2011-08-10 23:57:04 PDT
Comment on attachment 103585 [details]
Patch

Work in progress, comments on approach welcome.
Comment 3 Ben Wells 2011-08-11 00:17:43 PDT
Created attachment 103589 [details]
Test case showing fill and fillRect in all modes with transforms and clips.
Comment 4 WebKit Review Bot 2011-08-11 00:25:01 PDT
Comment on attachment 103585 [details]
Patch

Attachment 103585 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9346107

New failing tests:
canvas/philip/tests/2d.shadow.enable.y.html
fast/canvas/canvas-composite-alpha.html
fast/canvas/canvas-composite.html
canvas/philip/tests/2d.shadow.enable.x.html
Comment 5 WebKit Review Bot 2011-08-11 01:25:32 PDT
Comment on attachment 103585 [details]
Patch

Attachment 103585 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9346121

New failing tests:
canvas/philip/tests/2d.shadow.enable.y.html
fast/canvas/canvas-composite-alpha.html
fast/canvas/canvas-composite.html
canvas/philip/tests/2d.shadow.enable.x.html
Comment 6 Ben Wells 2011-08-15 18:22:12 PDT
Created attachment 103989 [details]
Patch
Comment 7 James Robinson 2011-08-15 18:49:14 PDT
Comment on attachment 103989 [details]
Patch

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

Getting there!

> LayoutTests/fast/canvas/canvas-composite-transformclip.html:76
> +        if (alpha) {
> +          context.fillStyle = 'rgba(255,64,0,0.5)';
> +        } else {
> +          context.fillStyle = 'rgba(255,64,0,1)';

I know that the existing compositing tests use red, but could you alter this test to use blue+green or something instead?  We typically try to avoid using red in our pixel test expectations.  fast/canvas/canvas-composite.html was imported, so we just left it as is, but for new tests we shouldn't have red.

> LayoutTests/platform/chromium/test_expectations.txt:3647
> +// Need rebaselining after canvas composite mode fixes
> +BUGWK66036 MAC WIN GPU : fast/canvas/canvas-composite.html = IMAGE
> +BUGWK66036 MAC WIN GPU : fast/canvas/canvas-composite-transformclip.html = IMAGE

i would expect that these tests need new baselines for chromium linux as well, but i don't see any on this patch. did you forget to git add the new -expected.png files?

> Source/WebCore/ChangeLog:10
> +        Canvas fill and fillRect with SourceIn, DestinationIn, SourceOut, DestinationAtop and Copy have errors
> +        https://bugs.webkit.org/show_bug.cgi?id=66036
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/canvas/canvas-composite-transformclip.html
> +
> +        * html/canvas/CanvasRenderingContext2D.cpp:

could you please explain what you are doing here? ideally there would be some description of what the canvas 2d globalCompositeOperation semantics are, how they are different from what the raster engines we use (CoreGraphics, skia, cairo, etc) expect, and how this approach bridges the gap

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1505
> +    canvasRect = state().m_transform.inverse().mapRect(canvasRect);
> +
> +    GraphicsContext* c = drawingContext();
> +    c->clearRect(canvasRect);

rather than trying to invert the current transform, can you just temporarily set the transform to identity?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1543
> +    OwnPtr<ImageBuffer> buffer =  ImageBuffer::create(bufferRect.size());

nit: extra space after the =

unfortunately it looks like this will not actually propagate the accelerated-ness of the ImageBuffer in the way you'd expect. to make this work properly, you'll need to add an interface to ImageBuffer to query its accelerated state and pass that in. you should probably also patch in the same color space, although i'm pretty sure all canvas ImageBuffers use the default (ColorSpaceDeviceRGB)
Comment 8 James Robinson 2011-08-15 18:49:47 PDT
Stephen, after your ImageBuffer stuff lands will passing a RenderingMode=Accelerated to ImageBuffer::create() be sufficient to make it accelerated in chromium?
Comment 9 Stephen White 2011-08-15 18:52:34 PDT
(In reply to comment #8)
> Stephen, after your ImageBuffer stuff lands will passing a RenderingMode=Accelerated to ImageBuffer::create() be sufficient to make it accelerated in chromium?

That's the plan, yes.  I haven't tested it with anything but the main canvas ImageBuffer so far, though.
Comment 10 Ben Wells 2011-08-15 21:32:26 PDT
Created attachment 104003 [details]
Patch updated for comments
Comment 11 Ben Wells 2011-08-15 21:35:21 PDT
Comment on attachment 104003 [details]
Patch updated for comments

I think I've addressed all the feedback. Two things I am not sure about:
- is isAccelerated the right way to test if a buffer is accelerated? There seem to be other ways scattered through the code (e.g. defines)
- the ColorSpace attribute passed into ImageBuffer at creation is ignored, I couldn't see a way to query this and use it when creating a new buffer. ColorSpaceDeviceRGB seems to be used throughout this file and also the rest of the canvas stuff.
Comment 12 James Robinson 2011-08-16 13:53:36 PDT
(In reply to comment #11)
> (From update of attachment 104003 [details])
> I think I've addressed all the feedback. Two things I am not sure about:
> - is isAccelerated the right way to test if a buffer is accelerated? There seem to be other ways scattered through the code (e.g. defines)

This is scattershot right now, but senorblanco has been working on unifying things.  After the patch on https://bugs.webkit.org/show_bug.cgi?id=66251 lands, ImageBuffer::isAccelerated() should do the right thing all the time.

> - the ColorSpace attribute passed into ImageBuffer at creation is ignored, I couldn't see a way to query this and use it when creating a new buffer. ColorSpaceDeviceRGB seems to be used throughout this file and also the rest of the canvas stuff.

Yeah - I think we always use that color space for canvas.
Comment 13 James Robinson 2011-08-16 14:16:56 PDT
Comment on attachment 104003 [details]
Patch updated for comments

nitpick: it kind of sucks that fast/canvas/canvas-composite-transformclip.html overflows, it means that we can't see the full results in the -expected.png and that we have native scrollbars, which makes expectations harder to deal with.  Can you possibly rejuggle things so we can see all of the results without any overflow?  Maybe cut down on the padding or something?

The code looks pretty good to me.  I think we should hold off until https://bugs.webkit.org/show_bug.cgi?id=66251 is resolved to land this, so I'll let anyone else who is interested comment.
Comment 14 Ben Wells 2011-08-16 21:28:10 PDT
Created attachment 104144 [details]
Patch
Comment 15 Ben Wells 2011-08-16 21:32:10 PDT
Comment on attachment 104144 [details]
Patch

Test shrunk down, code fixed up for acceleration and cleaned up a little.
Comment 16 James Robinson 2011-08-16 21:38:43 PDT
Comment on attachment 104144 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1508
> +    RenderingMode renderMode = canvas()->shouldAccelerate(bufferRect.size()) ? Accelerated : Unaccelerated;

this seems kind of weird - why are we asking the canvas if we should accelerate instead of checking if our current ImageBuffer is accelerated?  I think we should always try to match the accelerated state of the temporary buffer with the accelerated state of the existing buffer to try to cut down on the amount of gpu/cpu copies.
Comment 17 Ben Wells 2011-08-16 22:03:51 PDT
Created attachment 104148 [details]
Patch
Comment 18 Ben Wells 2011-08-16 22:14:14 PDT
Comment on attachment 104148 [details]
Patch

Buffer's accelerated state now matches canvas buffer's accelerated state.
Comment 19 James Robinson 2011-08-17 17:38:28 PDT
Comment on attachment 104148 [details]
Patch

Looks great!  R=me. Thanks for cleaning this up.

Can you file a follow-up bug to handle the rest of the draw calls properly (I think we currently composite incorrectly for strokes and all text operations)
Comment 20 WebKit Review Bot 2011-08-17 19:14:28 PDT
Comment on attachment 104148 [details]
Patch

Clearing flags on attachment: 104148

Committed r93275: <http://trac.webkit.org/changeset/93275>
Comment 21 WebKit Review Bot 2011-08-17 19:14:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Ben Wells 2011-08-22 23:18:26 PDT
*** Bug 48291 has been marked as a duplicate of this bug. ***
Comment 23 Ben Wells 2011-08-22 23:21:44 PDT
*** Bug 39177 has been marked as a duplicate of this bug. ***
Comment 24 Tom Hudson 2011-09-07 09:00:50 PDT
Created attachment 106588 [details]
Webpage capture showing errors

Several of the modes affected by this patch render incorrectly in Chromium, both in M15 tip-of-tree and in stable released M13, on screen as well as when printed.
In attached PDF, see source-in, source-out, destination-in, and copy; the destination object (blue rectangle) appears in the upper left quadrant of the output image, although it should not.

Reading the code suggests that there's likely to be another bug with gradients and other complex fills, since the current transform is applied to the path, and then rendered in identity space.
Comment 25 Ben Wells 2011-09-07 16:50:25 PDT
(In reply to comment #24)
> Created an attachment (id=106588) [details]
> Webpage capture showing errors
> 
> Several of the modes affected by this patch render incorrectly in Chromium, both in M15 tip-of-tree and in stable released M13, on screen as well as when printed.
> In attached PDF, see source-in, source-out, destination-in, and copy; the destination object (blue rectangle) appears in the upper left quadrant of the output image, although it should not.
> 
> Reading the code suggests that there's likely to be another bug with gradients and other complex fills, since the current transform is applied to the path, and then rendered in identity space.

I think the attached PDF is a capture of the test case above. This test case applies a clip to the canvas:

        context.beginPath();
        context.moveTo(1100, 1900);
        context.lineTo(1500, 1900);
        context.lineTo(1500, 2200);
        context.lineTo(1100, 2200);
        context.clip();

This clip is applied between drawing the 'destination' (the blue stuff) and drawing the 'source' (the red stuff). After transforms are taken into account, the clip should be stopping the source from being drawn at all in the top left corner of the boxes.

So I don't think this is a bug, but the desired behavior. 

The original bugs I fixed were:
* copy mode didn't clear at all
* SourceIn, SourceOut, DestIn and DestAtop modes had pixelation / artifacts at the boundard of the source as can be seen here: http://trac.webkit.org/export/92323/trunk/LayoutTests/platform/chromium-win/fast/canvas/canvas-composite-expected.png
Comment 26 Tom Hudson 2011-09-09 08:45:58 PDT
Ben's right, I'd missed the clip operation in the test case.
Comment 27 Stephen White 2011-09-12 11:51:35 PDT
One problem that seems to have been introduced by this change:  even in small canvases, a large filled rect with one of those transparency modes will cause a large ImageBuffer allocation.  See the test case in http://crbug.com/95178.

(There is an additional problem that I introduced which causes a crash, which is why that bug was dup'ed with another, but I think even if that is fixed, this will still cause large allocations).
Comment 28 Jeff Timanus 2011-09-12 14:00:24 PDT
(In reply to comment #27)
> One problem that seems to have been introduced by this change:  even in small canvases, a large filled rect with one of those transparency modes will cause a large ImageBuffer allocation.  See the test case in http://crbug.com/95178.

In relation to constructing a large ImageBuffer, the code in CanvasRenderingContext2D::fillAndDisplayTransparencyElsewhere will crash when ImageBuffer::create fails.  This is the tip of the call-stack using the test case from the above-mentioned crbug.

 	chrome.dll!WTF::OwnPtr<WebCore::ImageBuffer>::operator->()  Line 63 + 0x2f bytes	C++
>	chrome.dll!WebCore::CanvasRenderingContext2D::fillAndDisplayTransparencyElsewhere<WebCore::Path>(const WebCore::Path & area={...})  Line 1516 + 0xa bytes	C++
 	chrome.dll!WebCore::CanvasRenderingContext2D::fill()  Line 914 + 0xf bytes	C++
 	chrome.dll!WebCore::CanvasRenderingContext2DInternal::fillCallback(const v8::Arguments & args={...})  Line 525	C++




> 
> (There is an additional problem that I introduced which causes a crash, which is why that bug was dup'ed with another, but I think even if that is fixed, this will still cause large allocations).
Comment 29 Ben Wells 2011-09-12 15:41:13 PDT
I'll look at the crash, and also at optimizing the image buffer creation. It should create a buffer the size of which is the intersection of the target canvas and the path being filled.
Comment 30 Tom Hudson 2011-09-13 05:57:06 PDT
(In reply to comment #29)
> I'll look at the crash, and also at optimizing the image buffer creation. It should create a buffer the size of which is the intersection of the target canvas and the path being filled.

Not necessarily? The way I read the spec ("Without a clipping region, these operators act on the whole bitmap with every operation."), the buffer always needs to be as large as the target canvas.
Comment 31 Ben Wells 2011-09-14 01:17:23 PDT
(In reply to comment #30)
> (In reply to comment #29)
> Not necessarily? The way I read the spec ("Without a clipping region, these operators act on the whole bitmap with every operation."), the buffer always needs to be as large as the target canvas.

Yes, that's correct, the operator acts over the whole canvas. The buffer is only an intermediate thing however, and is used in conjunction with a clear operation over the whole canvas. The clear does the area outside the bounding rect of the fill, and the buffer does the bounding rect of the fill.
Comment 32 Eric Seidel (no email) 2011-09-19 10:51:32 PDT
*** Bug 48297 has been marked as a duplicate of this bug. ***
Comment 33 Ben Wells 2011-09-30 03:20:35 PDT
*** Bug 61470 has been marked as a duplicate of this bug. ***