Force bleed avoidance for some background image rendering

Force bleed avoidance for some background image rendering

When an element has both a background color and a background image, or multiple background images, the
back-most color is intended to come through when the image has transparency. But
when the element also has a border radius there may be bleeding among the background colors at the border, even for opaque images, due to the layers being clipped separately and then composited.

This patch checks for the specific situation and applies the necessary
bleed avoidance strategy.

R=fmalita@chromium.org
BUG=581616
Committed: https://crrev.com/46a5c2196e3746c376c1bdf75a8339732cd373b0
Cr-Commit-Position: refs/heads/master@{#376543}

[Stephen Chennney, 2016-02-05 21:43:59]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-05 21:46:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675823002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675823002/1

[commit-bot: I haz the power, 2016-02-06 02:36:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-06 02:36:32]

Dry run: This issue passed the CQ dry run.

[Stephen Chennney, 2016-02-09 20:59:25]

schenney@chromium.org changed reviewers:
+ jbroman@chromium.org

[Stephen Chennney, 2016-02-09 20:59:26]

Any takers to review this?

[jbroman, 2016-02-09 22:06:21]

lgtm

https://codereview.chromium.org/1675823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/BoxDecorationData.cpp (right):

https://codereview.chromium.org/1675823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxDecorationData.cpp:50: const
ComputedStyle& boxStyle = *layoutBox.style();
nit: *layoutBox.style() equivalent to layoutBox.styleRef(), which seems to be
very slightly preferred elsewhere?

[f(malita), 2016-02-09 22:07:39]

https://codereview.chromium.org/1675823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/BoxDecorationData.cpp (right):

https://codereview.chromium.org/1675823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxDecorationData.cpp:55: if
(backgroundColor.alpha() && boxStyle.hasBackgroundImage() && hasBorderRadius) {
I think one can stack background layers arbitrarily, even multiple images - so
only testing the background color may not be enough.  E.g., will this work for
two stacked non-opaque images when the background color is unspecified?

Also, the background painter stops painting when it encounters the first opaque
layer
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
 So if the top image is known to be opaque, we shouldn't need this extra layer
(which is not cheap).

So maybe a better precondition would be something along these lines:

  if (has_border_radius && bg_has_multiple_layers && !top_layer_is_opaque) ...

[f(malita), 2016-02-09 22:10:44]

It'd be a good idea to also try a CT10k run with this, to make sure we're not
introducing some major rasterization regression (saveLayers suck).

[Stephen Chennney, 2016-02-12 19:57:28]

On 2016/02/09 22:07:39, f(malita) wrote:
>
https://codereview.chromium.org/1675823002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/BoxDecorationData.cpp (right):
> 
>
https://codereview.chromium.org/1675823002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/BoxDecorationData.cpp:55: if
> (backgroundColor.alpha() && boxStyle.hasBackgroundImage() && hasBorderRadius)
{
> I think one can stack background layers arbitrarily, even multiple images - so
> only testing the background color may not be enough.  E.g., will this work for
> two stacked non-opaque images when the background color is unspecified?
> 
> Also, the background painter stops painting when it encounters the first
opaque
> layer
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
>  So if the top image is known to be opaque, we shouldn't need this extra layer
> (which is not cheap).
> 
> So maybe a better precondition would be something along these lines:
> 
>   if (has_border_radius && bg_has_multiple_layers && !top_layer_is_opaque) ...

Cluster telemetry in progress.

I think you're right about the multiple images and top layer opacity, but we
would need to use the same opacity check as in BoxPainter::paintFillLayer, i.e.
BoxPainter::isFillLayerOpaque. That requires the image repeat in XY, so the
opaque avatar in the original bug report doesn't seem to be considered opaque.
(I was wondering how the bug manifested when that avatar is opaque.)

But I don't think it matters if we have multiple layers. We are seeing the
problem here with just one layer.

The testing will need to be expanded to cover all these cases.

[Stephen Chennney, 2016-02-12 22:00:16]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-12 22:02:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675823002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675823002/20001

[Stephen Chennney, 2016-02-12 22:06:25]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-12 22:06:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675823002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675823002/40001

[Stephen Chennney, 2016-02-12 22:07:54]

Cluster telemetry is testing the new version. The older version has no real net
perf effect.

https://codereview.chromium.org/1675823002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxDecorationData.cpp (right):

https://codereview.chromium.org/1675823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxDecorationData.cpp:58: if
(boxStyle.hasBackgroundImage() && hasBorderRadius) {
Have to make sure we have the background image before testing for opaque.

https://codereview.chromium.org/1675823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxDecorationData.cpp:61: if
(!BoxPainter::isFillLayerOpaque(layoutBox.style()->backgroundLayers(),
layoutBox))
Have to use the same test as used in BoxPainter.

[commit-bot: I haz the power, 2016-02-12 22:11:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-12 22:11:45]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Stephen Chennney, 2016-02-15 14:40:06]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-15 14:40:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675823002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675823002/60001

[commit-bot: I haz the power, 2016-02-15 16:03:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-15 16:03:27]

Dry run: This issue passed the CQ dry run.

[Stephen Chennney, 2016-02-16 19:50:06]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-16 19:50:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675823002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675823002/80001

[Stephen Chennney, 2016-02-16 19:51:45]

I think this is now ready to land. fmalita@, could you give your OK.

I added the logic to deal with images over images and they were indeed a
problem, fixed by this patch.

Cluster telemetry runs show no net effect. The variance across pages is huge,
but there is no correlation with background images so I presume that's noise.

[commit-bot: I haz the power, 2016-02-16 21:00:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-16 21:00:02]

Dry run: This issue passed the CQ dry run.

[f(malita), 2016-02-19 20:31:39]

On 2016/02/16 19:51:45, Stephen Chenney wrote:
> I think this is now ready to land. fmalita@, could you give your OK.
> 
> I added the logic to deal with images over images and they were indeed a
> problem, fixed by this patch.
> 
> Cluster telemetry runs show no net effect. The variance across pages is huge,
> but there is no correlation with background images so I presume that's noise.

Thanks for looking at image stacking, LGTM.

[Stephen Chennney, 2016-02-19 20:32:18]

Description was changed from

==========
Force bleed avoidance for some background image rendering

When an element has both a background color and a background image, the
color is intended to come through when the image has transparency. But
when the element also has a border radius there may be bleeding of the
background at the border, even for opaque images, due to the color and
image being clipped separately and then composited.

This patch checks for the specific situation and applies the necessary
bleed avoidance strategy.

R=fmalita@chromium.org
BUG=581616
==========

to

==========
Force bleed avoidance for some background image rendering

When an element has both a background color and a background image, or multiple
background images, the
back-most color is intended to come through when the image has transparency. But
when the element also has a border radius there may be bleeding among the
background colors at the border, even for opaque images, due to the layers being
clipped separately and then composited.

This patch checks for the specific situation and applies the necessary
bleed avoidance strategy.

R=fmalita@chromium.org
BUG=581616
==========

[Stephen Chennney, 2016-02-19 20:32:46]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2016-02-19 20:32:46]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1675823002/#ps80001
(title: "Image over image test")

[commit-bot: I haz the power, 2016-02-19 20:34:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675823002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675823002/80001

[commit-bot: I haz the power, 2016-02-19 20:52:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-19 20:52:35]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[Stephen Chennney, 2016-02-19 21:08:48]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2016-02-19 21:09:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675823002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675823002/80001

[commit-bot: I haz the power, 2016-02-19 21:49:11]

Description was changed from

==========
Force bleed avoidance for some background image rendering

When an element has both a background color and a background image, or multiple
background images, the
back-most color is intended to come through when the image has transparency. But
when the element also has a border radius there may be bleeding among the
background colors at the border, even for opaque images, due to the layers being
clipped separately and then composited.

This patch checks for the specific situation and applies the necessary
bleed avoidance strategy.

R=fmalita@chromium.org
BUG=581616
==========

to

==========
Force bleed avoidance for some background image rendering

When an element has both a background color and a background image, or multiple
background images, the
back-most color is intended to come through when the image has transparency. But
when the element also has a border radius there may be bleeding among the
background colors at the border, even for opaque images, due to the layers being
clipped separately and then composited.

This patch checks for the specific situation and applies the necessary
bleed avoidance strategy.

R=fmalita@chromium.org
BUG=581616
==========

[commit-bot: I haz the power, 2016-02-19 21:49:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-19 21:50:27]

Description was changed from

==========
Force bleed avoidance for some background image rendering

When an element has both a background color and a background image, or multiple
background images, the
back-most color is intended to come through when the image has transparency. But
when the element also has a border radius there may be bleeding among the
background colors at the border, even for opaque images, due to the layers being
clipped separately and then composited.

This patch checks for the specific situation and applies the necessary
bleed avoidance strategy.

R=fmalita@chromium.org
BUG=581616
==========

to

==========
Force bleed avoidance for some background image rendering

When an element has both a background color and a background image, or multiple
background images, the
back-most color is intended to come through when the image has transparency. But
when the element also has a border radius there may be bleeding among the
background colors at the border, even for opaque images, due to the layers being
clipped separately and then composited.

This patch checks for the specific situation and applies the necessary
bleed avoidance strategy.

R=fmalita@chromium.org
BUG=581616

Committed: https://crrev.com/46a5c2196e3746c376c1bdf75a8339732cd373b0
Cr-Commit-Position: refs/heads/master@{#376543}
==========

[commit-bot: I haz the power, 2016-02-19 21:50:28]

Patchset 5 (id:??) landed as
https://crrev.com/46a5c2196e3746c376c1bdf75a8339732cd373b0
Cr-Commit-Position: refs/heads/master@{#376543}