Bug 66036 – Canvas fill and fillRect with SourceIn, DestinationIn, SourceOut, DestinationAtop and Copy have errors

• Component: New Bugs
• Status: RESOLVED FIXED

[Ben Wells]

Canvas compositing modes SourceIn, DestinationIn, SourceOut, DestinationAtop and Copy have errors

[Ben Wells]

Created attachment 103585 [details]
Patch

[Ben Wells]

Comment on attachment 103585 [details]
Patch

Work in progress, comments on approach welcome.

[Ben Wells]

Created attachment 103589 [details]
Test case showing fill and fillRect in all modes with transforms and clips.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ben Wells]

Created attachment 103989 [details]
Patch

[James Robinson]

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)

[James Robinson]

Stephen, after your ImageBuffer stuff lands will passing a RenderingMode=Accelerated to ImageBuffer::create() be sufficient to make it accelerated in chromium?

[Stephen White]

(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.

[Ben Wells]

Created attachment 104003 [details]
Patch updated for comments

[Ben Wells]

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.

[James Robinson]

(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.

[James Robinson]

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.

[Ben Wells]

Created attachment 104144 [details]
Patch

[Ben Wells]

Comment on attachment 104144 [details]
Patch

Test shrunk down, code fixed up for acceleration and cleaned up a little.

[James Robinson]

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.

[Ben Wells]

Created attachment 104148 [details]
Patch

[Ben Wells]

Comment on attachment 104148 [details]
Patch

Buffer's accelerated state now matches canvas buffer's accelerated state.

[James Robinson]

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)

[WebKit Review Bot]

Comment on attachment 104148 [details]
Patch

Clearing flags on attachment: 104148

Committed r93275: <http://trac.webkit.org/changeset/93275>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ben Wells]

*** Bug 48291 has been marked as a duplicate of this bug. ***

[Ben Wells]

*** Bug 39177 has been marked as a duplicate of this bug. ***

[Tom Hudson]

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.

[Ben Wells]

(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

[Tom Hudson]

Ben's right, I'd missed the clip operation in the test case.

[Stephen White]

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).

[Jeff Timanus]

(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).

[Ben Wells]

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.

[Tom Hudson]

(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.

[Ben Wells]

(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.

[Eric Seidel (no email)]

*** Bug 48297 has been marked as a duplicate of this bug. ***

[Ben Wells]

*** Bug 61470 has been marked as a duplicate of this bug. ***