Compute minimum repaint and show them in overlay

Compute minimum repaint and show them in overlay

For repaint tests, compute minimum repaint by comparing pixel dump
before and after repaintTest() and output repaint in layout test result.

In overlay, visualize the minimum repaint in expected results.

In this way, when making repaint change and checking actual results,
we can easily discover under-repaint bugs if we see any solid black
area not covered by repaint rects.

This CL depends on chromium-side CL https://codereview.chromium.org/301243022.

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

[Xianzhu, 2014-06-03 00:40:31]

There would be many repaint layout tests needing rebaseline. Will update
TestExpectations after running on try bots.

[Dirk Pranke, 2014-06-03 01:28:52]

I defer this to enne as well.

[enne (OOO), 2014-06-03 19:36:09]

Unless I'm missing something, this is not the minimum repaint rects, it's the
minimum *observed* repaint rects.  If a repaint test incorrectly missed an
invalidation, thus causing pixels not to change, then there would be no yellow
rect around it, and likewise no repaint rect in the output.

It's not clear to me how this catches any additional problems.

[Xianzhu, 2014-06-03 20:40:48]

On 2014/06/03 19:36:09, enne wrote:
> Unless I'm missing something, this is not the minimum repaint rects, it's the
> minimum *observed* repaint rects.  If a repaint test incorrectly missed an
> invalidation, thus causing pixels not to change, then there would be no yellow
> rect around it, and likewise no repaint rect in the output.
> 
> It's not clear to me how this catches any additional problems.

The overlay shows the baseline minimum repaint and the new repaint rects. We'll
discover under-repaints if the new repaint rects don't cover the baseline
minimum repaint.

This is how it works in details:
1. Create a baseline of current observed minimum repaint rects (like other
layout test expectations, we treat them as correct);
2. Change blink repaint code;
3. Run layout tests with the changed code. Suppose there is a under-repaint bug
in the changed code
  - If we support partial tile updates, the new minimum repaint will not match
the baseline minimum repaint and the layout test will fail;
  - If we don't support partial tile updates, the layout test may succeed and
the under-repaint bug may be not easily discovered from the repaint test output
even with pixel result. However, with the overlay visualizing new repaint rects
and the baseline minimum repaint, we can see the under-repaint.

Actually I'm using this patch and found it useful in my current blink repaint
optimization work. With it I discovered a border repainting bug of my changed
code.

[enne (OOO), 2014-06-03 23:23:53]

This will only show a repaint *if* the resulting inflated paint rectangle covers
something that the invalidation rectangle doesn't.  It seems like if you were
going to do something like this that you should do something that invalidates
the world to generate the final pixels, rather than depending on repaint
inflation that may change its implementation.

Can you convince me that this should go in the text output vs just being a
visual hint for people running tests locally?

The other worry that I have here is that displayAsyncThen is slow (issue
http://crbug.com/377575).  My initial intuition there is that this is slow
because it's copying around bitmaps.  If instead displayAsyncThen just waited
until a commit happened, I think things would be faster.  Your code forces that
bitmap to be copied via capturePixelsAsyncThen *and* adds another
capturePixelsAsyncThen for all cases.  Not understanding that bug makes me
reluctant to extend this further.

[Xianzhu, 2014-06-04 00:17:09]

On 2014/06/03 23:23:53, enne wrote:
> This will only show a repaint *if* the resulting inflated paint rectangle
covers
> something that the invalidation rectangle doesn't.

Actually it doesn't depend on inflated paint rectangle but needs minimal-repaint
baseline generated on a baseline system which we think it's correct.

However, thank you for the idea below. With it we don't need to generate a
minimal-repaint baseline in the text output, but can generate minimal-repaints
at any time even if the invalidation rects are incorrect.

> It seems like if you were
> going to do something like this that you should do something that invalidates
> the world to generate the final pixels, rather than depending on repaint
> inflation that may change its implementation.
> 
> Can you convince me that this should go in the text output vs just being a
> visual hint for people running tests locally?

I'm convinced by you instead :) As with full invalidation we can generate
minimal-repaint anytime, we can make this a tool usable by developers but not
necessary in the layout test pipeline. Will work on this.

> The other worry that I have here is that displayAsyncThen is slow (issue
> http://crbug.com/377575).  My initial intuition there is that this is slow
> because it's copying around bitmaps.  If instead displayAsyncThen just waited
> until a commit happened, I think things would be faster.  Your code forces
that
> bitmap to be copied via capturePixelsAsyncThen *and* adds another
> capturePixelsAsyncThen for all cases.  Not understanding that bug makes me
> reluctant to extend this further.

[enne (OOO), 2014-06-04 17:03:20]

On 2014/06/04 00:17:09, Xianzhu wrote:
> On 2014/06/03 23:23:53, enne wrote:
> > This will only show a repaint *if* the resulting inflated paint rectangle
> covers
> > something that the invalidation rectangle doesn't.
> 
> Actually it doesn't depend on inflated paint rectangle but needs
minimal-repaint
> baseline generated on a baseline system which we think it's correct.

Your patch doesn't use baselines, as far as I can see? Are you talking about how
you would want it to work in the future?

[Xianzhu, 2014-06-04 17:22:53]

On 2014/06/04 17:03:20, enne wrote:
> On 2014/06/04 00:17:09, Xianzhu wrote:
> > On 2014/06/03 23:23:53, enne wrote:
> > > This will only show a repaint *if* the resulting inflated paint rectangle
> > covers
> > > something that the invalidation rectangle doesn't.
> > 
> > Actually it doesn't depend on inflated paint rectangle but needs
> minimal-repaint
> > baseline generated on a baseline system which we think it's correct.
> 
> Your patch doesn't use baselines, as far as I can see? Are you talking about
how
> you would want it to work in the future?

My plan was to add [NeedToRebaseline]s into TestExpectations in this patch to
generate minimal repaint baselines. However, only after the chromium side change
submitted could I generate the list of test needing rebaseline on try bots, so
the baseline is not included yet (#2).

In the patch, repaint_overlay.py extracts the minimal repaint from the expected
layout result which in the plan was to include the minimal repaint baseline.

Locally I have generated baselines to test my repaint optimization change.

However, using your full-repaint method, we don't need to generate minimal
repaint baselines in advance, but can generate them at any time, so this can be
a local tool.

[Xianzhu, 2014-06-05 17:01:14]

PTAL.

Now use the full repaint method to calculate minimum repaint. Don't need to
rebaseline text expectations. Used as a local tool.

"How to use" is in fast/repaint/text-based-repaint-minimum-repaint.js. The
feature can be discovered by hovering mouse on "Show minimum repaint" or
"Highlight under-repaint" in the overlay page.

[enne (OOO), 2014-06-05 22:13:43]

I'm much more comfortable checking in something like this as a developer tool as
a first step, especially with all the bug caveats that you have in TODOs.

Thanks for the change for the full invalidation.  I think that just makes a lot
more sense to me.

https://codereview.chromium.org/315443002/diff/120001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/resources/text-based-repaint-minimum-repaint.js
(right):

https://codereview.chromium.org/315443002/diff/120001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/resources/text-based-repaint-minimum-repaint.js:36:
function runRepaintTest()
I'm not super happy with all the duplication here.  Is it possible to reuse
text-based-repaint.js and just set some window.calculateMinimumRepaintRects =
true if you want this extra stuff? You'd have to have an if statement around
capturePixelsAsyncThen vs. displayAsyncThen, but I think it'd be better to have
these two in sync?  What do you think?

https://codereview.chromium.org/315443002/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/controllers/repaint_overlay.py (right):

https://codereview.chromium.org/315443002/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/controllers/repaint_overlay.py:208: function
draw_minimum_repaint() {
Why does this need to be a separate canvas?

[Xianzhu, 2014-06-05 22:51:06]

https://codereview.chromium.org/315443002/diff/120001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/resources/text-based-repaint-minimum-repaint.js
(right):

https://codereview.chromium.org/315443002/diff/120001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/resources/text-based-repaint-minimum-repaint.js:36:
function runRepaintTest()
On 2014/06/05 22:13:42, enne wrote:
> I'm not super happy with all the duplication here.  Is it possible to reuse
> text-based-repaint.js and just set some window.calculateMinimumRepaintRects =
> true if you want this extra stuff? You'd have to have an if statement around
> capturePixelsAsyncThen vs. displayAsyncThen, but I think it'd be better to
have
> these two in sync?  What do you think?

Agreed. Done.

https://codereview.chromium.org/315443002/diff/120001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/controllers/repaint_overlay.py (right):

https://codereview.chromium.org/315443002/diff/120001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/controllers/repaint_overlay.py:208: function
draw_minimum_repaint() {
On 2014/06/05 22:13:42, enne wrote:
> Why does this need to be a separate canvas?

This is to avoid redraw all the rects when flipping between expected rects and
actual_rects and toggling the minimum repaint rects.

[enne (OOO), 2014-06-06 19:55:17]

I think the code looks ok, but I am not 100% sure that the UI makes sense.  I am
honestly super confused by highlight under repaint vs show minimum repaint.  I
*think* after hitting it a bunch and reading the code that the button is a
shortcut to do several things?

I think what I'd maybe want would be flashing back and forth between repaint and
minimal rects (and nothing?) so that it's easier to see when they are
overlapping? (Or like opacity 0.5 on the image so that there more contrast with
rects?) This is probably ui bikeshedding on my part.

At any rate, I think this is landable as-is since it is an improvement and is a
useful tool, and UI could get improved later if somebody wants.

https://codereview.chromium.org/315443002/diff/160001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/resources/text-based-repaint.js (right):

https://codereview.chromium.org/315443002/diff/160001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/resources/text-based-repaint.js:5:
window.generateMinimumRepaint = true; // See comments about 'Minimum repaint'
below.
Default to false?

https://codereview.chromium.org/315443002/diff/160001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/controllers/repaint_overlay.py (right):

https://codereview.chromium.org/315443002/diff/160001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/controllers/repaint_overlay.py:197: +
'\\nMinimal:\\n' + dump_rects(minimum_repaint);
This doesn't respect the show diffs only checkbox.  Should it?

[enne (OOO), 2014-06-06 19:55:28]

PS lgtm

[Xianzhu, 2014-06-07 05:23:06]

On 2014/06/06 19:55:17, enne wrote:
> I think the code looks ok, but I am not 100% sure that the UI makes sense.  I
am
> honestly super confused by highlight under repaint vs show minimum repaint.  I
> *think* after hitting it a bunch and reading the code that the button is a
> shortcut to do several things?
> 

You are right. The button will unset 'show test', 'diff only' and set 'show
minimum repaint', 'use solid colors', so that if there any under-repaint will be
shown as solid black.  

> I think what I'd maybe want would be flashing back and forth between repaint
and
> minimal rects (and nothing?) so that it's easier to see when they are
> overlapping? (Or like opacity 0.5 on the image so that there more contrast
with
> rects?) This is probably ui bikeshedding on my part.
> 

I did treak the ui for my own sake but I'm not sure if it is preferable for
others :)

For now, there are following layers in the overlay page (from bottom to top):
- the test itself: default is on, can be switched off to make the other layers
more visible;
- minimum-repaint: black, opacity=1 (in the last patch), default is off, can be
switched on;
- expected/actual repaint rects: red/green opacity can be switched between 0.25
and 1, always on.

https://codereview.chromium.org/315443002/diff/160001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/resources/text-based-repaint.js (right):

https://codereview.chromium.org/315443002/diff/160001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/resources/text-based-repaint.js:5:
window.generateMinimumRepaint = true; // See comments about 'Minimum repaint'
below.
On 2014/06/06 19:55:16, enne wrote:
> Default to false?

Sorry, I uploaded the version for local testing the feature.

Fixed.

https://codereview.chromium.org/315443002/diff/160001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/controllers/repaint_overlay.py (right):

https://codereview.chromium.org/315443002/diff/160001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/controllers/repaint_overlay.py:197: +
'\\nMinimal:\\n' + dump_rects(minimum_repaint);
On 2014/06/06 19:55:16, enne wrote:
> This doesn't respect the show diffs only checkbox.  Should it?

No. As we don't have minimum repaint baseline, there is also no diff for it.

[Xianzhu, 2014-06-16 21:45:24]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-06-16 21:45:41]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/315443002/200001

[commit-bot: I haz the power, 2014-06-17 00:17:40]

Change committed as 176257