When copying a region of a canvas to another region of the same canvas, drawImage does not work correctly if those regions have overlap. A reproduction code is here: https://jsfiddle.net/kfcea804/ Expected: Two green overlapping rectangles are drawn. Actual: A part of one rectangle is chipped (drawn as white, not green). The chipped region matches the area of the source region that is overwritten by the drawImage. I'm not sure, but the cause of this issue may be that WebKit does not taking a copy of the source image before the drawing operation, as opposed to what HTML Living Standard says: > When a canvas element is drawn onto itself, the drawing model requires the source to be copied before the image is drawn, so it is possible to copy parts of a canvas element onto overlapping parts of itself. https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-drawimage I am using Safari TP Release 75 (Safari 12.2, WebKit 14608.1.3.0.2) on macOS 10.14.3. I also checked that this issue is also reproduced with WebKit r241645.
Thank you for the report! Do you happen to know if this ever worked before in Safari?
Looking at the test case: The bug happens because of this setting: ctx.globalCompositeOperation = "copy"; Looking at the source code of CanvasRenderingContext2DBase::drawImage() and when if (state().globalComposite == CompositeCopy) { clearCanvas(); c->drawImageBuffer(*buffer, dstRect, srcRect, ImagePaintingOptions(state().globalComposite, state().globalBlend)); didDrawEntireCanvas(); } Calling clearCanvas() will clear the entire canvas rectangle. Because the test case clips to the destination rect(64, 64, 128, 128), clearCanvas() will only clear this rectangle. But this destination rectangle overlaps the source rectangle rect(0, 0, 128, 128). So the intersection rectangle (64, 64, 128, 128) is also cleared from the source rectangle. And this is why a white rectangle in the middle of the cnavas is drawn.
So we need to read out the source before clearing in this case?
<rdar://problem/48169601>
(In reply to Alexey Proskuryakov from comment #1) > Thank you for the report! Do you happen to know if this ever worked before > in Safari? No, I don't know it worked before.
Created attachment 362445 [details] Patch
Comment on attachment 362445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362445&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1665 > + if (&sourceCanvas == &canvasBase()) { > + auto imageBuffer = drawCanvasToBuffer(srcRect); > + clearCanvas(); > + c->drawImageBuffer(*imageBuffer, dstRect, { { }, srcRect.size() }, ImagePaintingOptions(state().globalComposite, state().globalBlend)); Should we look to see if srcRect and dstRect overlap and only do the copy in that case?
Comment on attachment 362445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362445&action=review >> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1665 >> + c->drawImageBuffer(*imageBuffer, dstRect, { { }, srcRect.size() }, ImagePaintingOptions(state().globalComposite, state().globalBlend)); > > Should we look to see if srcRect and dstRect overlap and only do the copy in that case? No. Because if srcRect and dstRect do not overlap, we will fall through the else-statement which will clearCanvas() before drawImageBuffer() . For example, the new test layout in this patch will show nothing if I change the above if statement to be: if (&sourceCanvas == &canvasBase() && srcRect.intersects(dstRect))
Comment on attachment 362445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362445&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1602 > +std::unique_ptr<ImageBuffer> CanvasRenderingContext2DBase::drawCanvasToBuffer(const FloatRect& srcRect) const I wonder if this should be called copyCanvasRegionToBuffer? > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1611 > + auto& canvas = downcast<HTMLCanvasElement>(canvasBase()); > + ImageBuffer* srcBuffer = canvas.buffer(); > + if (!srcBuffer) > + return nullptr; This can go first in the function to skip creating the dstBuffer if it fails.
Comment on attachment 362445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362445&action=review > LayoutTests/fast/canvas/canvas-drawImage-composite-copy.html:2 > + <canvas id="canvas" width="200" height="200"></canvas> We should probably make this a WPT and submit it back.
Created attachment 362522 [details] Patch
Comment on attachment 362445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362445&action=review >> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1602 >> +std::unique_ptr<ImageBuffer> CanvasRenderingContext2DBase::drawCanvasToBuffer(const FloatRect& srcRect) const > > I wonder if this should be called copyCanvasRegionToBuffer? I moved this function to ImageBuffer and I name it copyRectToBuffer(). So please have a look at the new patch. >> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1611 >> + return nullptr; > > This can go first in the function to skip creating the dstBuffer if it fails. You are right. After moving this function to ImageBuffer, I call it from CanvasRenderingContext2DBase::drawImage() from buffer->copyRectToBuffer(). We check nullability of buffer before this call. So this check is not needed anymore. >> LayoutTests/fast/canvas/canvas-drawImage-composite-copy.html:2 >> + <canvas id="canvas" width="200" height="200"></canvas> > > We should probably make this a WPT and submit it back. I will talk to Youenn Fablet about the process to get it as WPT.
Comment on attachment 362522 [details] Patch Clearing flags on attachment: 362522 Committed r241840: <https://trac.webkit.org/changeset/241840>
All reviewed patches have been landed. Closing bug.