Fixing color bleeding when the canvas is stretched.

Fixing color bleeding when the canvas is stretched.

When the canvas is stretched by zooming the browser window,
the interpolation filter may cause color bleeding. We fix this
by half-pixel padding before paint invalidation when necessary.

BUG=654700
Committed: https://crrev.com/4bdb40f764d2fc3ac7f4e34da68424e84b89968f
Cr-Commit-Position: refs/heads/master@{#427382}

[zakerinasab, 2016-10-21 15:18:38]

Patchset #1 (id:1) has been deleted

[zakerinasab, 2016-10-21 15:23:11]

Description was changed from

==========
Addressing color bleeding issue.



BUG=
==========

to

==========
Fixing color bleeding when the canvas is stretched.

When the canvas is stretched by zooming the browser window,
the interpolation filter may cause color bleeding. See the
bug description for an example.
Layout test cannot be generated for this CL as there is no
access to the browser zoom in JavaScript and scaling the
document element or canvas element cannot be used to reproduce 
this issue.

BUG=654700
==========

[zakerinasab, 2016-10-21 15:23:11]

zakerinasab@chromium.org changed reviewers:
+ junov@chromium.org, xidachen@chromium.org

[zakerinasab, 2016-10-21 15:23:47]

I uploaded the CL. I can't find a proper way to write the layout test though.

[Justin Novosad, 2016-10-21 15:56:03]

For the test, you should try using a canvas that is zoomed using CSS.

https://codereview.chromium.org/2438313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2438313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:335: if
(FloatRect(ro->contentBoxRect()).contains(srcRect) &&
"Contains" is not the right condition to determine whether the image is being
stretched. You could be stretching in one dimension and compressing in another,
there could also be a position offset due to padding.  What you should do here
is compare the width and height of the rects.

https://codereview.chromium.org/2438313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:337:
m_dirtyRect.expand(FloatSize(0.5, 0.5));
You should use inflate here.

[Justin Novosad, 2016-10-21 16:02:10]

Here is a fiddle that reproduces the bug: 
https://jsfiddle.net/3erkjboa/

[zakerinasab, 2016-10-21 16:03:56]

On 2016/10/21 16:02:10, Justin Novosad wrote:
> Here is a fiddle that reproduces the bug: 
> https://jsfiddle.net/3erkjboa/

It doesn't reproduce the bug on my Linux and Windows stations.

[xidachen, 2016-10-21 16:07:04]

On 2016/10/21 16:03:56, zakerinasab wrote:
> On 2016/10/21 16:02:10, Justin Novosad wrote:
> > Here is a fiddle that reproduces the bug: 
> > https://jsfiddle.net/3erkjboa/
> 
> It doesn't reproduce the bug on my Linux and Windows stations.

Try this then: https://jsfiddle.net/3erkjboa/1/

[Justin Novosad, 2016-10-21 16:35:25]

Just re-click the "run" button if you don't see the bug the first time.

[zakerinasab, 2016-10-21 17:55:22]

Description was changed from

==========
Fixing color bleeding when the canvas is stretched.

When the canvas is stretched by zooming the browser window,
the interpolation filter may cause color bleeding. See the
bug description for an example.
Layout test cannot be generated for this CL as there is no
access to the browser zoom in JavaScript and scaling the
document element or canvas element cannot be used to reproduce 
this issue.

BUG=654700
==========

to

==========
Fixing color bleeding when the canvas is stretched.

When the canvas is stretched by zooming the browser window,
the interpolation filter may cause color bleeding. We fix this
by half-pixel padding before paint invalidation when necessary.


BUG=654700
==========

[zakerinasab, 2016-10-21 17:55:36]

Layout test added.

[Justin Novosad, 2016-10-21 18:15:00]

https://codereview.chromium.org/2438313002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-stretch-color-bleeding.html
(right):

https://codereview.chromium.org/2438313002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/canvas-stretch-color-bleeding.html:10:
<canvas id='foo' width='100' height='100' style='width:500px;height:100px'>
</canvas>
make the style height smaller, then you will see why there is still a bug with
the "contains" logic in your current fix.

https://codereview.chromium.org/2438313002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/canvas-stretch-color-bleeding.html:15:
var colorData = ctx.getImageData(k, 50, 1, 1).data;
This is not a good test. The test will pass even without the current fix.  The
problem is not with the pixel values in the canvas, it is with how the values
are propagated to the display, which is not reflected by getImageData.  You
should use a ref test (with an -expected.html)

[zakerinasab, 2016-10-21 19:20:07]

New patch submitted.

https://codereview.chromium.org/2438313002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-stretch-color-bleeding.html
(right):

https://codereview.chromium.org/2438313002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/canvas-stretch-color-bleeding.html:10:
<canvas id='foo' width='100' height='100' style='width:500px;height:100px'>
</canvas>
On 2016/10/21 18:15:00, Justin Novosad wrote:
> make the style height smaller, then you will see why there is still a bug with
> the "contains" logic in your current fix.

Acknowledged.

https://codereview.chromium.org/2438313002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/canvas-stretch-color-bleeding.html:15:
var colorData = ctx.getImageData(k, 50, 1, 1).data;
On 2016/10/21 18:15:00, Justin Novosad wrote:
> This is not a good test. The test will pass even without the current fix.  The
> problem is not with the pixel values in the canvas, it is with how the values
> are propagated to the display, which is not reflected by getImageData.  You
> should use a ref test (with an -expected.html)

Right. Ref test doesn't seem to work too, as result png image seems to be a
snapshot from the canvas. PTAL at the new test file.

[zakerinasab, 2016-10-21 20:56:16]

New patch submitted.

[Justin Novosad, 2016-10-24 17:53:10]

https://codereview.chromium.org/2438313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2438313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:337:
m_dirtyRect.expand(FloatSize(0.5, 0.5));
Just use inflate

[zakerinasab, 2016-10-24 18:14:35]

New patch submitted.

https://codereview.chromium.org/2438313002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2438313002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:337:
m_dirtyRect.expand(FloatSize(0.5, 0.5));
On 2016/10/24 17:53:10, Justin Novosad wrote:
> Just use inflate

Done.

[Justin Novosad, 2016-10-25 14:30:15]

https://codereview.chromium.org/2438313002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2438313002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:337:
m_dirtyRect.inflate(0.25);
Are you sure 0.25 is always enough? Even at very large scale factors? 0.5 seems
more logical to me.

[zakerinasab, 2016-10-25 14:35:55]

On 2016/10/25 14:30:15, Justin Novosad wrote:
>
https://codereview.chromium.org/2438313002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):
> 
>
https://codereview.chromium.org/2438313002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:337:
> m_dirtyRect.inflate(0.25);
> Are you sure 0.25 is always enough? Even at very large scale factors? 0.5
seems
> more logical to me.

The highest zooming allowed by the browser is 500% for which half a pixel
inflation seems to work fine (I guess it results in rounding up the width to an
extra one pixel). We can go for one pixel inflation too.

[zakerinasab, 2016-10-25 14:36:21]

New patch submitted.

[Justin Novosad, 2016-10-25 15:14:03]

On 2016/10/25 14:35:55, zakerinasab wrote:
> On 2016/10/25 14:30:15, Justin Novosad wrote:
> >
>
https://codereview.chromium.org/2438313002/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2438313002/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:337:
> > m_dirtyRect.inflate(0.25);
> > Are you sure 0.25 is always enough? Even at very large scale factors? 0.5
> seems
> > more logical to me.
> 
> The highest zooming allowed by the browser is 500% for which half a pixel
> inflation seems to work fine (I guess it results in rounding up the width to
an
> extra one pixel). We can go for one pixel inflation too.

Yes, but in CSS (which is another way to repro the bug), there is no such limit.

lgtm

[zakerinasab, 2016-10-25 15:14:55]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2016-10-25 15:15:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 16:52:34]

Description was changed from

==========
Fixing color bleeding when the canvas is stretched.

When the canvas is stretched by zooming the browser window,
the interpolation filter may cause color bleeding. We fix this
by half-pixel padding before paint invalidation when necessary.


BUG=654700
==========

to

==========
Fixing color bleeding when the canvas is stretched.

When the canvas is stretched by zooming the browser window,
the interpolation filter may cause color bleeding. We fix this
by half-pixel padding before paint invalidation when necessary.


BUG=654700
==========

[commit-bot: I haz the power, 2016-10-25 16:52:35]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-10-25 17:16:34]

Description was changed from

==========
Fixing color bleeding when the canvas is stretched.

When the canvas is stretched by zooming the browser window,
the interpolation filter may cause color bleeding. We fix this
by half-pixel padding before paint invalidation when necessary.


BUG=654700
==========

to

==========
Fixing color bleeding when the canvas is stretched.

When the canvas is stretched by zooming the browser window,
the interpolation filter may cause color bleeding. We fix this
by half-pixel padding before paint invalidation when necessary.

BUG=654700

Committed: https://crrev.com/4bdb40f764d2fc3ac7f4e34da68424e84b89968f
Cr-Commit-Position: refs/heads/master@{#427382}
==========

[commit-bot: I haz the power, 2016-10-25 17:16:35]

Patchset 7 (id:??) landed as
https://crrev.com/4bdb40f764d2fc3ac7f4e34da68424e84b89968f
Cr-Commit-Position: refs/heads/master@{#427382}