putImageData leaves visible artifacts on retina display
Reported by
ber...@gmail.com,
May 6 2016
|
||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2726.0 Safari/537.36 Steps to reproduce the problem: 1. Go to http://bertfreudenberg.github.io/SqueakJS/test/putImageData.html 2. Notice gribblies left behind 3. Scroll page, notice gribblies go away The above is a test case I isolated, in production you can see the problem when dragging the window at http://bertfreudenberg.github.io/SqueakJS/demo/simple.html#fullscreen What is the expected behavior? Expect clean white rendering of the background. The canvas only has black and white pixels, there should be no gray. What went wrong? half-pixel gray artifacts show around the area affected by the putImageData rendering Did this work before? Yes was fine in Chrome 51, buggy in 49 Chrome version: 52.0.2726.0 Channel: canary OS Version: OS X 10.11.4 Flash Version: Shockwave Flash 22.0 r0 Same issue was fixed in webkit recently https://bugs.webkit.org/show_bug.cgi?id=156039 by enlarging the dirty area by 1 pixel on a retina screen
,
May 6 2016
Did bisect between range: 390063 (good) and 392063 (bad), and looks like it is due to this skia change: https://chromium.googlesource.com/skia.git/+log/ffc2aea3cb69..6536ae590e38 mtklein: could you take a look at those three CLs in that deps roll? Thank you.
,
May 13 2016
,
May 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0baf3521c90b2efda95e33c5ed26cc2c51423381 commit 0baf3521c90b2efda95e33c5ed26cc2c51423381 Author: xidachen <xidachen@chromium.org> Date: Wed May 18 00:33:30 2016 Inflate dirty rect by 1 pixel on a high DPI display On a high dpi display, putImageData leaves visible artifacts around the affected area's border. This CL fixes it in the way that when there is antialias involved, the dirty rect is inflated by 1 pixel. Locally we can see this bug goes away with this fix on a retina display. Note that this bug doesn't repro with our layout test. So a manual test is added to prevent regression. BUG= 609820 Review-Url: https://codereview.chromium.org/1983863002 Cr-Commit-Position: refs/heads/master@{#394286} [add] https://crrev.com/0baf3521c90b2efda95e33c5ed26cc2c51423381/third_party/WebKit/ManualTests/canvas-ClearRect-hidpi.html [modify] https://crrev.com/0baf3521c90b2efda95e33c5ed26cc2c51423381/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp [modify] https://crrev.com/0baf3521c90b2efda95e33c5ed26cc2c51423381/third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h [modify] https://crrev.com/0baf3521c90b2efda95e33c5ed26cc2c51423381/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp [modify] https://crrev.com/0baf3521c90b2efda95e33c5ed26cc2c51423381/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.h
,
May 18 2016
,
May 18 2016
The patch in HTMLCanvasElement.cpp looks to me as if it would inflate m_dirtyRect every time didDraw() is called. With many draw calls it would get larger and larger. Wouldn't it be more correct to inflate rect before merging it into m_dirtyRect?
,
May 18 2016
Yes, that makes total sense. A new patch will be out soon.
,
May 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0e9fa080a3438b3e4e8f2c525eaa43fff1393125 commit 0e9fa080a3438b3e4e8f2c525eaa43fff1393125 Author: xidachen <xidachen@chromium.org> Date: Wed May 18 15:39:14 2016 Let dirty rect inflate after its unite with the affected area In our previous CL: https://codereview.chromium.org/1983863002/, we let the dirty rect inflate by one pixel. However, this makes the dirty rect keeps growing. Instead, we should inflate the dirty rect after computing its intersection with the rect, which is the actual affected area. BUG= 609820 TBR=junov@chromium.org Review-Url: https://codereview.chromium.org/1994613002 Cr-Commit-Position: refs/heads/master@{#394429} [modify] https://crrev.com/0e9fa080a3438b3e4e8f2c525eaa43fff1393125/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
,
May 18 2016
This will still inflate m_dirtyRect every time didDraw() is called. You need something like
m_dirtyRect.unite(rect.inflate(1));
that is, call inflate() on `rect` not `m_dirtyRect`. (but I'm not sure if it's kosher to modify `rect` or if you need a temp copy)
,
May 18 2016
I believe that at each new frame, the m_dirtyRect gets reset to (0, 0, 0, 0).
,
May 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c3d88062790e63b468cbe1b0a672d244baa33f48 commit c3d88062790e63b468cbe1b0a672d244baa33f48 Author: xidachen <xidachen@chromium.org> Date: Wed May 18 21:06:50 2016 Inflate rect instead of m_dirtyRect in HTMLCanvasElement::didDraw This is a clean up CL for https://codereview.chromium.org/1994613002/, we should not inflate m_dirtyRect because it will accumulate that inflation. BUG= 609820 Review-Url: https://codereview.chromium.org/1991563003 Cr-Commit-Position: refs/heads/master@{#394545} [modify] https://crrev.com/c3d88062790e63b468cbe1b0a672d244baa33f48/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
,
May 18 2016
Looks good :) |
||||
►
Sign in to add a comment |
||||
Comment 1 by dtapu...@chromium.org
, May 6 2016