Impl-side painting for virtual/regionbasedmulticol/ layout tests

Impl-side painting for virtual/regionbasedmulticol/ layout tests

From the "sad about opacity in reftests" department...

R=chrishtr@chromium.org
BUG=381919
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187216

[chrishtr, 2014-12-16 02:23:37]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2014-12-16 02:23:37]

lgtm

[commit-bot: I haz the power, 2014-12-16 02:23:51]

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

[commit-bot: I haz the power, 2014-12-16 03:33:48]

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

[mstensho (USE GERRIT), 2014-12-18 22:11:15]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2014-12-18 22:11:15]

I think it would have been better to fix the refs rather than turning them into
pngs and render tree dumps. I'll see what I can do about that.

[enne (OOO), 2014-12-18 22:23:34]

On 2014/12/18 at 22:11:15, mstensho wrote:
> I think it would have been better to fix the refs rather than turning them
into pngs and render tree dumps. I'll see what I can do about that.

No argument from me.  I looked at these for an hour or so and couldn't sort it
out, so if you can fix them, I would happily review that change.  I didn't do
that lightly.

I'm converting a lot of tests, many of which have opacity errors.  I've fixed
some ref tests, where there were straightforward ways to fix them by changing
values.  However, the Skia team has historically shown not much interest in
fixing off-by-one-bit blending issues when blending takes slightly different
codepaths.  So, the only way to fix these things is to attempt to make both
versions blend in exactly the same way, which is always complex and often
impossible.

[mstensho (USE GERRIT), 2014-12-19 14:39:40]

On 2014/12/18 22:23:34, enne wrote:
> On 2014/12/18 at 22:11:15, mstensho wrote:
> > I think it would have been better to fix the refs rather than turning them
> into pngs and render tree dumps. I'll see what I can do about that.
> 
> No argument from me.  I looked at these for an hour or so and couldn't sort it
> out, so if you can fix them, I would happily review that change.  I didn't do
> that lightly.

Yeah, tricky stuff, indeed.

> I'm converting a lot of tests, many of which have opacity errors.  I've fixed
> some ref tests, where there were straightforward ways to fix them by changing
> values.  However, the Skia team has historically shown not much interest in
> fixing off-by-one-bit blending issues when blending takes slightly different
> codepaths.  So, the only way to fix these things is to attempt to make both
> versions blend in exactly the same way, which is always complex and often
> impossible.

I've managed to fix 2 out of 4 tests so far. It was all about making the ref
establish the same layers as in the test.

The 2 remaining tests don't seem to establish any sub-layers at all
(--show-composited-layer-borders). They seem to do the exact same thing in the
test and in the ref, minus multicolumnedness. I think I need to learn exactly
what impl side painting does (apart from what the documentation of
--enable-impl-side-painting says). Could layer squashing make a difference?

I find it quite unacceptable that it's rocket surgery to write opacity reftests.
Surely we need to fix this? If it cannot be fixed in code, maybe we need to
allow minor pixel differences for testing? Maybe the tests that need it could
say <script>internals.settings.allowMinorPixelDifferences()</script> or
something?

[enne (OOO), 2014-12-19 18:42:14]

> I've managed to fix 2 out of 4 tests so far. It was all about making the ref
establish the same layers as in the test.

Excellent! Thank you!
 
> The 2 remaining tests don't seem to establish any sub-layers at all
(--show-composited-layer-borders). They seem to do the exact same thing in the
test and in the ref, minus multicolumnedness. I think I need to learn exactly
what impl side painting does (apart from what the documentation of
--enable-impl-side-painting says). Could layer squashing make a difference?

The only difference I can think of is that impl-side painting goes through
SkPicture instead of being written to an SkBitmap directly, and maybe there's
some optimization or something along the way that changes the Skia layers that
are used for opacity.

Impl-side painting presentation link, if that's helpful:
https://docs.google.com/a/chromium.org/presentation/d/1nPEC4YRz-V1m_TsGB0pK3m...

> I find it quite unacceptable that it's rocket surgery to write opacity
reftests. Surely we need to fix this? If it cannot be fixed in code, maybe we
need to allow minor pixel differences for testing? Maybe the tests that need it
could say <script>internals.settings.allowMinorPixelDifferences()</script> or
something?

I and other folks have looked at fixing this in code, and it's a giant pile of
work (probably impossible, and even if not, easily regressible over time) for
what amounts to imperceptible pixel differences.  Don't get me wrong; this is a
pragmatic attitude I'm expressing here.  Opacity is intensely frustrating and
this state of the world is really no good, but it's measured against what it
would take to fix it and what gains would be had by spending that time.

It might be possible to do something like an internals.setting, but I'm not sure
how to plumb that through.  WebTestProxyBase is what does the screenshotting,
and I think the harness itself is what does the image comparisons, but there's
no easy way to get from A to B (I think).  Maybe a test expectation [
MinorPixelDifferences ] might be a more straightforward approach?

[mstensho (USE GERRIT), 2015-01-07 13:29:26]

I ended up with a different solution, which fixes everything, and is simple and
... magical. :-P

See https://codereview.chromium.org/839793003/