Check rect under-invalidations

Check rect under-invalidations

There are two modes:
- Using RuntimeEnabledFeatures::slimmingPaintUnderInvalidationChecking.
  When the feature is enabled, for each paint, changed pixels without
  invalidation are logged and marked red.
- In layout tests, when tracking paint invalidation, content_shell will
  crash if there are any changed pixels without invalidation.

The method is: for each GraphicsLayer, after each paint, save the
painted picture. After another paint, rasterize the old and new pictures
into bitmaps and compare the bitmaps. If any pixel changed but not
covered by paint invalidations between the paints, take action (log,
mark as red or crash).

BUG=617799, 619242
R=chrishtr@chromium.org, pdr@chromium.org

Committed: https://crrev.com/43b0c4f23351a0bbfaccfea8d0ce10fec85977db
Cr-Commit-Position: refs/heads/master@{#399331}

[Xianzhu, 2016-06-09 17:10:43]

Description was changed from

==========
Check rect under-invalidations

BUG=617799
==========

to

==========
Check rect under-invalidations

There are two modes:
- Using RuntimeEnabledFeatures::slimmingPaintUnderInvalidationChecking.
  When the feature is enabled, for each paint, changed pixels without
  invalidation are logged and marked red.
- In layout tests, when tracking paint invalidation, content_shell will
  crash if there are any changed pixels without invalidation.

The method is: for each GraphicsLayer, after each paint, save the
painted picture. After another paint, rasterize the old and new pictures
into bitmaps and compare the bitmaps. If any pixel changed but not
covered by paint invalidations between the paints, take action (log,
mark as red or crash).

BUG=617799
==========

[Xianzhu, 2016-06-09 17:25:43]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org

[pdr., 2016-06-09 19:21:44]

https://codereview.chromium.org/2049263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/2049263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:376:
DisplayItemCacheSkipper cacheSkipper(context);
This change makes sense, but why was it needed for this patch?

https://codereview.chromium.org/2049263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:1270:
getPaintController().paintArtifact().replay(graphicsContext);
Why look at the pixels instead of just comparing the display item sizes and
types?

https://codereview.chromium.org/2049263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.h (right):

https://codereview.chromium.org/2049263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.h:214: bool
isTrackingOrCheckingPaintInvalidations() const
I worry that the checking part will be very slow on debug bots. Do we need this
to run the pixel verification continuously? I am wondering if we could just do
the pixel verification manually using the flag.

[Xianzhu, 2016-06-09 20:47:45]

https://codereview.chromium.org/2049263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/2049263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:376:
DisplayItemCacheSkipper cacheSkipper(context);
On 2016/06/09 19:21:44, pdr. wrote:
> This change makes sense, but why was it needed for this patch?

This is not needed for this patch. Reverted.

https://codereview.chromium.org/2049263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:1270:
getPaintController().paintArtifact().replay(graphicsContext);
On 2016/06/09 19:21:44, pdr. wrote:
> Why look at the pixels instead of just comparing the display item sizes and
> types?

Several reasons:

1. For incremental invalidation which just invalidate part of an display item,
we expect that only the incremental part of the display item changed. It seems
not feasible to check if a display item didn't change outside a certain rect
without rasterizing it.

2. We need to trust display item's visualRect to check if a changed display item
is covered by the paint invalidation rects. Because visual rects and paint
invalidation rects are actually from the same code path, we'll miss bugs about
paint invalidation rect calculation.

3. Sometimes we need rasterization invalidation when the display item itself
doesn't change but its location changes. We can find display items whose
location changed and check if they are covered by paint invalidation rects, but
this will have the same problem as 2.

https://codereview.chromium.org/2049263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.h (right):

https://codereview.chromium.org/2049263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.h:214: bool
isTrackingOrCheckingPaintInvalidations() const
On 2016/06/09 19:21:44, pdr. wrote:
> I worry that the checking part will be very slow on debug bots. Do we need
this
> to run the pixel verification continuously? I am wondering if we could just do
> the pixel verification manually using the flag.

If the flag is not enabled, we do pixel verification only in repaint tests which
explicitly track paint invalidations. Most of the tests just track paint
invalidations for just 1 or 2 paintings.

Compared timings of running layout tests under fast/repaint with and without
this patch with debug builds:

With this patch:
12:59:58.361 15624 PER TEST TIME IN TESTSHELL (seconds):
12:59:58.361 15624   Median:           0.590
12:59:58.361 15624   Mean:             0.523
12:59:58.361 15624   90th percentile:  0.776
12:59:58.361 15624   99th percentile:  1.118
12:59:58.361 15624   Standard dev:     0.045
slowest: fast/repaint/4774354.html took 1.5 seconds

Without this patch:
13:35:42.968 23271 PER TEST TIME IN TESTSHELL (seconds):
13:35:42.968 23271   Median:           0.615
13:35:42.968 23271   Mean:             0.540
13:35:42.968 23271   90th percentile:  0.793
13:35:42.968 23271   99th percentile:  1.157
13:35:42.968 23271   Standard dev:     0.052
slowest: fast/repaint/4774354.html took 1.5 seconds

They are basically the same indicating that pixel verification contributes
little to the time.

[pdr., 2016-06-09 21:25:47]

On 2016/06/09 at 20:47:45, wangxianzhu wrote:
Thanks for checking those.


Do we need two distinct modes (the runtime flag and the internals calls to
start/stop tracking) or could they be combined behind
RuntimeEnabledFeatures::slimmingPaintUnderInvalidationChecking?

[Xianzhu, 2016-06-09 22:54:32]

On 2016/06/09 21:25:47, pdr. wrote:
> On 2016/06/09 at 20:47:45, wangxianzhu wrote:
> Thanks for checking those.
> 
> 
> Do we need two distinct modes (the runtime flag and the internals calls to
> start/stop tracking) or could they be combined behind
> RuntimeEnabledFeatures::slimmingPaintUnderInvalidationChecking?

Good question.

Just noticed that we can modify mutable run-time features in layout tests. In
the latest patch pixel verification is controlled by
RuntimeEnabledFeatures::slimmingPaintUnderInvalidationCheckingEnabled only.

[Xianzhu, 2016-06-09 22:56:03]

https://codereview.chromium.org/2049263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(left):

https://codereview.chromium.org/2049263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:371:
#endif
This is removed because it doesn't apply to the new client-side cache flag.

[pdr., 2016-06-09 22:56:36]

On 2016/06/09 at 22:54:32, wangxianzhu wrote:
> On 2016/06/09 21:25:47, pdr. wrote:
> > On 2016/06/09 at 20:47:45, wangxianzhu wrote:
> > Thanks for checking those.
> > 
> > 
> > Do we need two distinct modes (the runtime flag and the internals calls to
> > start/stop tracking) or could they be combined behind
> > RuntimeEnabledFeatures::slimmingPaintUnderInvalidationChecking?
> 
> Good question.
> 
> Just noticed that we can modify mutable run-time features in layout tests. In
the latest patch pixel verification is controlled by
RuntimeEnabledFeatures::slimmingPaintUnderInvalidationCheckingEnabled only.

New patch is great. LGTM here, but please give chrishtr a chance to look too

[Xianzhu, 2016-06-10 18:42:58]

chrishtr@ would you still like to take a look at the whole patch?

https://codereview.chromium.org/2049263003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/2049263003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:1305: static bool
pixelComponentsDiffer(int c1, int c2)
Modified the pixel verification algorithm by tolerating invisible differences
that may occur in gradients etc.

Ptal?

[chrishtr, 2016-06-10 18:47:03]

Look reasonable to me. Nice approach. I didn't review every line so defer to
pdr's LGTM.

https://codereview.chromium.org/2049263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.h (right):

https://codereview.chromium.org/2049263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.h:306: void
checkUnderPaintInvalidations(const SkPicture&);
checkPaintUnderInvalidations

[Xianzhu, 2016-06-10 18:52:14]

https://codereview.chromium.org/2049263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.h (right):

https://codereview.chromium.org/2049263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.h:306: void
checkUnderPaintInvalidations(const SkPicture&);
On 2016/06/10 18:47:02, chrishtr wrote:
> checkPaintUnderInvalidations

Done locally. Will upload with updated TestExpectations after the current try
run.

[Xianzhu, 2016-06-10 20:56:09]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-06-10 20:56:10]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2049263003/#ps180001
(title: "x")

[commit-bot: I haz the power, 2016-06-10 20:57:08]

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

[Xianzhu, 2016-06-10 21:24:07]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-06-10 21:24:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2049263003/#ps200001
(title: "x")

[commit-bot: I haz the power, 2016-06-10 21:24:54]

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

[commit-bot: I haz the power, 2016-06-10 22:50:22]

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

[commit-bot: I haz the power, 2016-06-10 22:50:23]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2016-06-11 00:36:48]

Description was changed from

==========
Check rect under-invalidations

There are two modes:
- Using RuntimeEnabledFeatures::slimmingPaintUnderInvalidationChecking.
  When the feature is enabled, for each paint, changed pixels without
  invalidation are logged and marked red.
- In layout tests, when tracking paint invalidation, content_shell will
  crash if there are any changed pixels without invalidation.

The method is: for each GraphicsLayer, after each paint, save the
painted picture. After another paint, rasterize the old and new pictures
into bitmaps and compare the bitmaps. If any pixel changed but not
covered by paint invalidations between the paints, take action (log,
mark as red or crash).

BUG=617799
==========

to

==========
Check rect under-invalidations

There are two modes:
- Using RuntimeEnabledFeatures::slimmingPaintUnderInvalidationChecking.
  When the feature is enabled, for each paint, changed pixels without
  invalidation are logged and marked red.
- In layout tests, when tracking paint invalidation, content_shell will
  crash if there are any changed pixels without invalidation.

The method is: for each GraphicsLayer, after each paint, save the
painted picture. After another paint, rasterize the old and new pictures
into bitmaps and compare the bitmaps. If any pixel changed but not
covered by paint invalidations between the paints, take action (log,
mark as red or crash).

BUG=617799,619242
==========

[commit-bot: I haz the power, 2016-06-11 00:40:19]

Description was changed from

==========
Check rect under-invalidations

There are two modes:
- Using RuntimeEnabledFeatures::slimmingPaintUnderInvalidationChecking.
  When the feature is enabled, for each paint, changed pixels without
  invalidation are logged and marked red.
- In layout tests, when tracking paint invalidation, content_shell will
  crash if there are any changed pixels without invalidation.

The method is: for each GraphicsLayer, after each paint, save the
painted picture. After another paint, rasterize the old and new pictures
into bitmaps and compare the bitmaps. If any pixel changed but not
covered by paint invalidations between the paints, take action (log,
mark as red or crash).

BUG=617799,619242
==========

to

==========
Check rect under-invalidations

There are two modes:
- Using RuntimeEnabledFeatures::slimmingPaintUnderInvalidationChecking.
  When the feature is enabled, for each paint, changed pixels without
  invalidation are logged and marked red.
- In layout tests, when tracking paint invalidation, content_shell will
  crash if there are any changed pixels without invalidation.

The method is: for each GraphicsLayer, after each paint, save the
painted picture. After another paint, rasterize the old and new pictures
into bitmaps and compare the bitmaps. If any pixel changed but not
covered by paint invalidations between the paints, take action (log,
mark as red or crash).

BUG=617799,619242
R=chrishtr@chromium.org, pdr@chromium.org

Committed: https://crrev.com/43b0c4f23351a0bbfaccfea8d0ce10fec85977db
Cr-Commit-Position: refs/heads/master@{#399331}
==========

[commit-bot: I haz the power, 2016-06-11 00:40:20]

Patchset 12 (id:??) landed as
https://crrev.com/43b0c4f23351a0bbfaccfea8d0ce10fec85977db
Cr-Commit-Position: refs/heads/master@{#399331}

[Xianzhu, 2016-06-11 00:48:57]

Patchset #14 (id:260001) has been deleted

[Xianzhu, 2016-06-11 00:49:05]

Patchset #13 (id:240001) has been deleted