CTM-independent BackgroundBleedBackgroundOverBorder implementation

CTM-independent BackgroundBleedBackgroundOverBorder implementation

The BackgroundBleedBackgroundOverBorder backdrop bleed avoidance
strategy draws opaque backgrounds (shrunk to the inner border edge) on
top of the border. It also attempts to expand the inner border edge by
one pixel in device space, to mitigate anti-aliasing blending artifacts
along the common inner-border/outer-background edge.

This approach is fragile and doesn't work reliably with impl-side
painting and slimming paint, because the final device space transform
is unknown at recording time.

The CL updates BackgroundBleedBackgroundOverBorder to fill the inner
border rrect completely. Since this is no longer a device-space
dependent operation, it now produces correct results under arbitrary
rasterization-time transformations. It also becomes usable with
slimming paint.

BUG=445598
R=junov@chromium.org,chrishtr@chromium.org,robertphillips@google.com

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193660

[f(malita), 2015-04-13 20:07:21]

This has no effect for non-slimming-paint tests, but fixes a bunch of slimming
paint backdrop bleed regressions:
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

Local Telemetry benchmarks show no impact on rasterization time.

[Justin Novosad, 2015-04-13 20:24:53]

On 2015/04/13 20:07:21, f(malita) wrote:
> This has no effect for non-slimming-paint tests, but fixes a bunch of slimming
> paint backdrop bleed regressions:
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
> 
> Local Telemetry benchmarks show no impact on rasterization time.

LGTM! Thanks for getting rid of innerBorderAdjustment.

As far as perf is concerned, this will probably be a bit slower in some cases
and a bit faster in others. Not worth over-analyzing because correctness comes
before performance anyways.

[chrishtr, 2015-04-13 20:40:11]

https://codereview.chromium.org/1088613002/diff/40001/Source/core/paint/BoxDe...
File Source/core/paint/BoxDecorationData.cpp (right):

https://codereview.chromium.org/1088613002/diff/40001/Source/core/paint/BoxDe...
Source/core/paint/BoxDecorationData.cpp:43: AffineTransform ctm =
context->getCTM();
Please factor this code into borderObscuresBackgroundEdge so it's not called if
RuntimeEnabledFeatures::slimmingPaintEnabled()

https://codereview.chromium.org/1088613002/diff/40001/Source/core/paint/BoxDe...
Source/core/paint/BoxDecorationData.cpp:64: if
(!RuntimeEnabledFeatures::slimmingPaintEnabled() &&
borderObscuresBackgroundEdge(*layoutBox.style(), contextScaling))
Why not just do it unconditionally? It's not slimming-paint specific.

https://codereview.chromium.org/1088613002/diff/40001/Source/core/paint/BoxDe...
Source/core/paint/BoxDecorationData.cpp:68: return
BackgroundBleedBackgroundOverBorder;
I know it's not blocking slimming paint, but is it feasible to get rid of this
one too? If so add a TODO(fmalita)?

[f(malita), 2015-04-13 21:17:59]

https://codereview.chromium.org/1088613002/diff/40001/Source/core/paint/BoxDe...
File Source/core/paint/BoxDecorationData.cpp (right):

https://codereview.chromium.org/1088613002/diff/40001/Source/core/paint/BoxDe...
Source/core/paint/BoxDecorationData.cpp:43: AffineTransform ctm =
context->getCTM();
On 2015/04/13 20:40:11, chrishtr wrote:
> Please factor this code into borderObscuresBackgroundEdge so it's not called
if
> RuntimeEnabledFeatures::slimmingPaintEnabled()

Done.

https://codereview.chromium.org/1088613002/diff/40001/Source/core/paint/BoxDe...
Source/core/paint/BoxDecorationData.cpp:64: if
(!RuntimeEnabledFeatures::slimmingPaintEnabled() &&
borderObscuresBackgroundEdge(*layoutBox.style(), contextScaling))
On 2015/04/13 20:40:11, chrishtr wrote:
> Why not just do it unconditionally? It's not slimming-paint specific.

Not sure I follow here: BackgroundBleedShrinkBackground doesn't work well with
SP (and is also fragile with impl-side) because both the heuristic used to
determine whether it's usable and its impl are dependent on CTM knowledge.

If everything turns out as planned, I hope we can remove it when switching to SP
as there is no good way to make it work reliably.

https://codereview.chromium.org/1088613002/diff/40001/Source/core/paint/BoxDe...
Source/core/paint/BoxDecorationData.cpp:68: return
BackgroundBleedBackgroundOverBorder;
On 2015/04/13 20:40:11, chrishtr wrote:
> I know it's not blocking slimming paint, but is it feasible to get rid of this
> one too? If so add a TODO(fmalita)?

(are you mixing these fine enum names maybe?)

This one will have to stay, because it's the only cheap bleed avoidance
workaround we have that is now SP/impl-side proof.

The remaining SP regressions are due to limitations of the
BackgroundBleedClipBackground fallback. I have a follow up to address that, but
it relies on using an extra layer - which is not cheap. The only way it can
stick is if BackgroundBleedClipBackground remains an uncommon fallback, with
most borders handled by BackgroundBleedBackgroundOverBorder.

[chrishtr, 2015-04-13 21:26:50]

On 2015/04/13 at 21:17:59, fmalita wrote:
>
https://codereview.chromium.org/1088613002/diff/40001/Source/core/paint/BoxDe...
> File Source/core/paint/BoxDecorationData.cpp (right):
> 
>
https://codereview.chromium.org/1088613002/diff/40001/Source/core/paint/BoxDe...
> Source/core/paint/BoxDecorationData.cpp:43: AffineTransform ctm =
context->getCTM();
> On 2015/04/13 20:40:11, chrishtr wrote:
> > Please factor this code into borderObscuresBackgroundEdge so it's not called
if
> > RuntimeEnabledFeatures::slimmingPaintEnabled()
> 
> Done.
> 
>
https://codereview.chromium.org/1088613002/diff/40001/Source/core/paint/BoxDe...
> Source/core/paint/BoxDecorationData.cpp:64: if
(!RuntimeEnabledFeatures::slimmingPaintEnabled() &&
borderObscuresBackgroundEdge(*layoutBox.style(), contextScaling))
> On 2015/04/13 20:40:11, chrishtr wrote:
> > Why not just do it unconditionally? It's not slimming-paint specific.
> 
> Not sure I follow here: BackgroundBleedShrinkBackground doesn't work well with
SP (and is also fragile with impl-side) because both the heuristic used to
determine whether it's usable and its impl are dependent on CTM knowledge.
> 
> If everything turns out as planned, I hope we can remove it when switching to
SP as there is no good way to make it work reliably.

Sorry, I phrased badly. I meant why not just delete the whole heuristic in this
patch, since it is not SP-specific. If it won't work for non-SP then it won't
work for SP either, so why not just rip it out?

> 
>
https://codereview.chromium.org/1088613002/diff/40001/Source/core/paint/BoxDe...
> Source/core/paint/BoxDecorationData.cpp:68: return
BackgroundBleedBackgroundOverBorder;
> On 2015/04/13 20:40:11, chrishtr wrote:
> > I know it's not blocking slimming paint, but is it feasible to get rid of
this
> > one too? If so add a TODO(fmalita)?
> 
> (are you mixing these fine enum names maybe?)
> 
> This one will have to stay, because it's the only cheap bleed avoidance
workaround we have that is now SP/impl-side proof.

I'm asking because it's nice to remove special cases from the paint code. I
don't know all the details of BackgroundBleedBackgroundOverBorder, is it really
hard to express it directly without putting the painter into a special mode?

> 
> The remaining SP regressions are due to limitations of the
BackgroundBleedClipBackground fallback. I have a follow up to address that, but
it relies on using an extra layer - which is not cheap. The only way it can
stick is if BackgroundBleedClipBackground remains an uncommon fallback, with
most borders handled by BackgroundBleedBackgroundOverBorder.

Why are these remaining regressions SP-specific? Oh, maybe I see: could it be
that the SP regressions remaining are avoided for non-SP in this patch because
BackgroundBleedShrinkBackground is still present for non-SP?

[f(malita), 2015-04-13 21:35:43]

On 2015/04/13 21:26:50, chrishtr wrote:
> On 2015/04/13 at 21:17:59, fmalita wrote:
>
https://codereview.chromium.org/1088613002/diff/40001/Source/core/paint/BoxDe...
> > Source/core/paint/BoxDecorationData.cpp:64: if
> (!RuntimeEnabledFeatures::slimmingPaintEnabled() &&
> borderObscuresBackgroundEdge(*layoutBox.style(), contextScaling))
> > On 2015/04/13 20:40:11, chrishtr wrote:
> > > Why not just do it unconditionally? It's not slimming-paint specific.
> > 
> > Not sure I follow here: BackgroundBleedShrinkBackground doesn't work well
with
> SP (and is also fragile with impl-side) because both the heuristic used to
> determine whether it's usable and its impl are dependent on CTM knowledge.
> > 
> > If everything turns out as planned, I hope we can remove it when switching
to
> SP as there is no good way to make it work reliably.
> 
> Sorry, I phrased badly. I meant why not just delete the whole heuristic in
this
> patch, since it is not SP-specific. If it won't work for non-SP then it won't
> work for SP either, so why not just rip it out?

I agree it should be removed eventually, but would prefer to tread carefully
here and only remove it after all other pieces are in place, to avoid
regressions.

(it does work most of the time, and deleting it now would definitely trigger
several non-SP layout test regressions)

> > The remaining SP regressions are due to limitations of the
> BackgroundBleedClipBackground fallback. I have a follow up to address that,
but
> it relies on using an extra layer - which is not cheap. The only way it can
> stick is if BackgroundBleedClipBackground remains an uncommon fallback, with
> most borders handled by BackgroundBleedBackgroundOverBorder.
> 
> Why are these remaining regressions SP-specific? Oh, maybe I see: could it be
> that the SP regressions remaining are avoided for non-SP in this patch because
> BackgroundBleedShrinkBackground is still present for non-SP?

Exactly: BackgroundBleedShrinkBackground handles most non-SP borders currently.

[chrishtr, 2015-04-13 21:42:01]

lgtm

[f(malita), 2015-04-13 21:53:06]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2015-04-13 21:53:06]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1088613002/#ps60001
(title: "review comments")

[commit-bot: I haz the power, 2015-04-13 21:53:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1088613002/60001

[commit-bot: I haz the power, 2015-04-13 23:49:30]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193660