New issue
Advanced search Search tips

Issue 843606 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task



Sign in to add a comment

Refactor raster invalidation

Project Member Reported by wangxianzhu@chromium.org, May 16 2018

Issue description

For now raster invalidation has two levels:
1. chunk level: issues raster invalidation for display items within each paint chunk, for
  - full/incremental invalidation marked before painting;
  - display item recorders;
  - appearing and disappearing display items.
  The code is in PaintController.

2. layer level: issues raster invalidation for paint chunks within each composited layer, for
  - paint property changes of paint chunks;
  - paint chunk recorders;
  - appearing and disappearing paint chunks.
 The code is in CompositedLayerRasterInvalidator

The design is suitable for the current paint and compositing phase separation, but it has additional requirement for chunk ids to match old and new chunks for both levels. This sometimes needs us to create additional paint chunks to keep chunk ids stable across document cycles, to avoid unnecessary whole chunk raster invalidation just because of change of chunk ids. https://chromium-review.googlesource.com/c/chromium/src/+/1040876 shows examples of the cases that we need more paint chunks.

Discussed with pdr in the patch, and we propose the following refactoring method:

- Move level 1 raster invalidation code into CompositedLayerRasterInvalidator (which will be renamed to RasterInvalidator). This requires us to also refactor PaintController::CommitNewDisplayItems() and PaintController::FinishCycle() to keep both the new and old paint artifacts until FinishCycle().

- Do level 1 raster invalidation for a chunk only if level 2 won't invalidate the chunk. This avoids unnecessary level 1 work.

- For appearing and disappearing paint chunks, search for their display items across paint chunks to find out the real appearing and disappearing display items and issue raster invalidations, to avoid unnecessary full chunk raster invalidation on chunk id changes.




 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/43505bb6d6033a65a51935ff2d64d2f97484ff50

commit 43505bb6d6033a65a51935ff2d64d2f97484ff50
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Tue Jun 12 20:29:25 2018

[CI] compositing/CompositedLayerRasterInvalidator -> paint/RasterInvalidator

This is the first step of refactoring raster invalidator, before
moving raster invalidation code out of PaintController into
paint/RasterInvalidator.

Bug:  843606 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I0bd4b98b2ccd0e0bc6b7c87255ce81c33115a16b
Reviewed-on: https://chromium-review.googlesource.com/1097507
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566553}
[modify] https://crrev.com/43505bb6d6033a65a51935ff2d64d2f97484ff50/third_party/blink/renderer/platform/BUILD.gn
[modify] https://crrev.com/43505bb6d6033a65a51935ff2d64d2f97484ff50/third_party/blink/renderer/platform/graphics/compositing/content_layer_client_impl.h
[modify] https://crrev.com/43505bb6d6033a65a51935ff2d64d2f97484ff50/third_party/blink/renderer/platform/graphics/graphics_layer.cc
[modify] https://crrev.com/43505bb6d6033a65a51935ff2d64d2f97484ff50/third_party/blink/renderer/platform/graphics/graphics_layer.h
[modify] https://crrev.com/43505bb6d6033a65a51935ff2d64d2f97484ff50/third_party/blink/renderer/platform/graphics/graphics_layer_test.cc
[rename] https://crrev.com/43505bb6d6033a65a51935ff2d64d2f97484ff50/third_party/blink/renderer/platform/graphics/paint/raster_invalidator.cc
[rename] https://crrev.com/43505bb6d6033a65a51935ff2d64d2f97484ff50/third_party/blink/renderer/platform/graphics/paint/raster_invalidator.h
[rename] https://crrev.com/43505bb6d6033a65a51935ff2d64d2f97484ff50/third_party/blink/renderer/platform/graphics/paint/raster_invalidator_test.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/35e3eed7609b25cc60f3b5e657eae9c11af6a17b

commit 35e3eed7609b25cc60f3b5e657eae9c11af6a17b
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Fri Jun 29 20:29:24 2018

[RasterInvalidation] Move raster invalidation code out of PaintController into RasterInvalidator

Previously raster invalidations for changed display items were generated
by PaintController during painting and recorded in
PaintChunksAndRasterInvalidations.

Now RasterInvalidator checks for changed paint chunks first, and if a
paint chunk is not fully invalidated, call DisplayItemRasterInvalidator::
Generate() to generate raster invalidations for changed display items
in the chunk.

Some other changes are needed to achieve that:
- We no longer need to save raster invalidations for display items
  along with paint artifact;
- Let RasterInvalidator own the previous paint artifact shared with
  PaintController) so that DisplayItemRasterInvalidator can compare old
  and new display items;
- Some updates of client and display item status previously done by
  PaintController::CommitNewDisplayItems() are now done after
  RasterInvalidator::Generate() in PaintController::FinishCycle().

Bug:  843606 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I06a9845aa10974252ea0590d91b32a81444684ae
Reviewed-on: https://chromium-review.googlesource.com/1104885
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571614}
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/WebKit/LayoutTests/flag-specific/enable-slimming-paint-v2/paint/invalidation/position/transform-absolute-child-expected.txt
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/WebKit/LayoutTests/flag-specific/enable-slimming-paint-v2/paint/invalidation/position/transform-relative-position-expected.txt
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/WebKit/LayoutTests/flag-specific/enable-slimming-paint-v2/paint/invalidation/transform/transform-replaced-shadows-expected.txt
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/WebKit/LayoutTests/paint/invalidation/position/fixed-scale-expected.txt
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/WebKit/LayoutTests/paint/invalidation/position/transform-absolute-child-expected.txt
[rename] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/WebKit/LayoutTests/paint/invalidation/position/transform-absolute-in-positioned-container-expected.txt
[rename] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/WebKit/LayoutTests/paint/invalidation/position/transform-relative-position-expected.txt
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/WebKit/LayoutTests/paint/invalidation/svg/filter-refresh-expected.txt
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/WebKit/LayoutTests/paint/invalidation/transform/transform-replaced-shadows-expected.txt
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/WebKit/LayoutTests/platform/mac/paint/invalidation/position/transform-absolute-in-positioned-container-expected.txt
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/WebKit/LayoutTests/platform/mac/paint/invalidation/position/transform-relative-position-expected.txt
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/core/editing/caret_display_item_client_test.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/core/paint/paint_and_raster_invalidation_test.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/core/paint/paint_and_raster_invalidation_test.h
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/core/paint/paint_controller_paint_test.h
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/core/paint/paint_layer_painter_test.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/BUILD.gn
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/compositing/content_layer_client_impl.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/compositing/content_layer_client_impl.h
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/compositing/paint_artifact_compositor.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/compositing/paint_artifact_compositor.h
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/compositing/paint_artifact_compositor_test.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/graphics_layer.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/graphics_layer_test.cc
[add] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/display_item_raster_invalidator.cc
[add] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/display_item_raster_invalidator.h
[add] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/display_item_raster_invalidator_test.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/paint_artifact.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/paint_artifact.h
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/paint_chunk_subset.h
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/paint_chunker.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/paint_chunker.h
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/paint_chunker_test.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/paint_controller.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/paint_controller.h
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/paint_controller_debug_data.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/paint_controller_test.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/paint_controller_test.h
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/paint_record_builder_test.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/raster_invalidation_tracking.h
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/raster_invalidator.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/raster_invalidator.h
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint/raster_invalidator_test.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint_invalidation_reason.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/graphics/paint_invalidation_reason.h
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/testing/test_paint_artifact.cc
[modify] https://crrev.com/35e3eed7609b25cc60f3b5e657eae9c11af6a17b/third_party/blink/renderer/platform/testing/test_paint_artifact.h

Status: Fixed (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/515036ccc2a632bd8fcf04e3939de5e89fd8ffe8

commit 515036ccc2a632bd8fcf04e3939de5e89fd8ffe8
Author: Daniel Bratell <bratell@opera.com>
Date: Mon Jul 02 14:14:26 2018

Don't convert IntRect -> FloatRect -> IntRect.

If EnclosingIntRect() is called on an IntRect it is first
converted to a FloatRect or DoubleRect since that is what
EnclosingIntRect() is used for. Then you (hopefully) get
the originally IntRect back.

This strange conversion wasn't obvious in the code because the
type of the IntRect was hidden by an |auto|. It was discovered in
jumbo builds because the compiler had access to both DoubleRect
and FloatRect and didn't know which one to use.

Bug:  843606 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I1ca799cc27da02e1c6615bf403bc190fec1d5cde
Reviewed-on: https://chromium-review.googlesource.com/1122219
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#571887}
[modify] https://crrev.com/515036ccc2a632bd8fcf04e3939de5e89fd8ffe8/third_party/blink/renderer/platform/graphics/paint/display_item_raster_invalidator.cc

Sign in to add a comment