Bug 156039 - putImageData leaves visible artifacts on retina display
Summary: putImageData leaves visible artifacts on retina display
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: zalan
URL: HTTPS://bertfreudenberg.github.io/Squ...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-30 13:30 PDT by Bert Freudenberg
Modified: 2016-04-05 21:19 PDT (History)
10 users (show)

See Also:


Attachments
Test reduction (727 bytes, text/html)
2016-03-31 14:24 PDT, zalan
no flags Details
Patch (4.11 KB, patch)
2016-03-31 21:50 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (4.69 KB, patch)
2016-04-01 16:01 PDT, zalan
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bert Freudenberg 2016-03-30 13:30:18 PDT
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
Comment 1 zalan 2016-03-31 14:24:45 PDT
Created attachment 275327 [details]
Test reduction
Comment 2 zalan 2016-03-31 21:06:49 PDT
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.
Comment 3 Radar WebKit Bug Importer 2016-03-31 21:07:05 PDT
<rdar://problem/25482243>
Comment 4 zalan 2016-03-31 21:50:11 PDT
Created attachment 275369 [details]
Patch
Comment 5 Brent Fulgham 2016-04-01 11:16:09 PDT
Comment on attachment 275369 [details]
Patch

r=me
Comment 6 zalan 2016-04-01 16:01:05 PDT
Created attachment 275439 [details]
Patch
Comment 7 zalan 2016-04-01 16:03:13 PDT
Comment on attachment 275439 [details]
Patch

Simon says r+.
Comment 8 WebKit Commit Bot 2016-04-01 16:04:10 PDT
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
Comment 9 zalan 2016-04-01 16:25:46 PDT
Committed r198958: <http://trac.webkit.org/changeset/198958>
Comment 10 Darin Adler 2016-04-03 12:29:28 PDT
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?
Comment 11 zalan 2016-04-05 14:39:24 PDT
(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.