New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 609820 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

putImageData leaves visible artifacts on retina display

Reported by ber...@gmail.com, May 6 2016

Issue description

UserAgent: 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
 
Components: -Blink Blink>Canvas
Cc: mtkl...@chormium.org
Status: Available (was: Unconfirmed)
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.
Cc: -mtkl...@chormium.org junov@chromium.org
Owner: xidac...@chromium.org
Status: Started (was: Available)
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Comment 6 by ber...@gmail.com, 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?
Yes, that makes total sense. A new patch will be out soon.
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Comment 9 by ber...@gmail.com, 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)
I believe that at each new frame, the m_dirtyRect gets reset to (0, 0, 0, 0).
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Comment 12 by ber...@gmail.com, May 18 2016

Looks good :)

Sign in to add a comment