Bug 66036

Created attachment 103585 [details] Patch

Comment on attachment 103585 [details] Patch Work in progress, comments on approach welcome.

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

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

Created attachment 103989 [details] Patch

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)

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

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

Created attachment 104003 [details] Patch updated for comments

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.

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

Created attachment 104144 [details] Patch

Comment on attachment 104144 [details] Patch Test shrunk down, code fixed up for acceleration and cleaned up a little.

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.

Created attachment 104148 [details] Patch

Comment on attachment 104148 [details] Patch Buffer's accelerated state now matches canvas buffer's accelerated state.

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 on attachment 104148 [details] Patch Clearing flags on attachment: 104148 Committed r93275: <http://trac.webkit.org/changeset/93275>

All reviewed patches have been landed. Closing bug.

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

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

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.

(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

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

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

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

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.

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

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

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

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