putImageData leaves visible artifacts on retina display

[Vanessa Freudenberg]

On a retina display, putImageData leaves gribblies around the affected area's border. Changing the page zoom clears these artifacts, proving that the actual canvas content is correct.

To reproduce I have isolated a simple test case here:
http://bertfreudenberg.github.io/SqueakJS/test/putImageData.html

In production this manifests e.g. when trying to move a window here:
https://bertfreudenberg.github.io/SqueakJS/demo/simple.html

Note that the gribblies do not show up when using pixelated image-rendering:
https://bertfreudenberg.github.io/SqueakJS/demo/simple.html#pixelated

[zalan]

Created attachment 275327 [details]
Test reduction

[zalan]

This should fix it. 
diff --git a/Source/WebCore/html/HTMLCanvasElement.cpp b/Source/WebCore/html/HTMLCanvasElement.cpp
index 0021486..d687335 100644
--- a/Source/WebCore/html/HTMLCanvasElement.cpp
+++ b/Source/WebCore/html/HTMLCanvasElement.cpp
@@ -314,9 +314,12 @@ void HTMLCanvasElement::didDraw(const FloatRect& rect)
             return;
 
         m_dirtyRect.unite(r);
+        if (drawingContext() && drawingContext()->shouldAntialias() && document().deviceScaleFactor() > 1) {
+            // Inflate dirty rect to cover antialiasing on image buffers where CSS px size != device px size.
+            m_dirtyRect.inflate(1 / document().deviceScaleFactor());
+        }
         ro->repaintRectangle(enclosingIntRect(m_dirtyRect));
     }
-
     notifyObserversCanvasChanged(rect);
 }

Patch is coming up soon.

[Radar WebKit Bug Importer]

<rdar://problem/25482243>

[zalan]

Created attachment 275369 [details]
Patch

[Brent Fulgham]

Comment on attachment 275369 [details]
Patch

r=me

[zalan]

Created attachment 275439 [details]
Patch

[zalan]

Comment on attachment 275439 [details]
Patch

Simon says r+.

[WebKit Commit Bot]

Comment on attachment 275439 [details]
Patch

Rejecting attachment 275439 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 275439, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/1083408

[zalan]

Committed r198958: <http://trac.webkit.org/changeset/198958>

[Darin Adler]

Comment on attachment 275439 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:314
> +            dirtyRect.inflate(1);

Why is 1 always correct? Shouldn’t this depend on the scale factor somehow?

[zalan]

(In reply to comment #10)
> Comment on attachment 275439 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275439&action=review
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:314
> > +            dirtyRect.inflate(1);
> 
> Why is 1 always correct? Shouldn’t this depend on the scale factor somehow?
If by scale factor, you mean 
1. pinch-zoom scale, then the answer is no. repaint rects are in document coordinates.
2. canvas scale by setting a different width/height on the canvas and the canvas element, then the answer is yes and that scaling happens in mapRect().  
3. retina vs. non-retina, the answer is yes but according to Simon it's better to inflate always with 1 canvas pixel since we don't control antialias painting behaviour.