Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2833883003:
Skip paint chunks with effectively invisible opacity. (Closed)
DescriptionSkip paint chunks with effectively invisible opacity.
For SPv2 we always paint effectively invisible content,
but during layerization we scrutinize effect node and
skip chunks that would be effectively invisible. This
simplifies code in SPv2 while still avoiding the
resource and processing overhead of creating, managing
and rastering a layer for that chunk.
BUG=713403
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2833883003
Cr-Commit-Position: refs/heads/master@{#468120}
Committed: https://chromium.googlesource.com/chromium/src/+/caaa5c77ce09501d9dd409bde61ae226e1768b7f
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rework effectively invisible opacity optimization to handle in PAC. #Patch Set 3 : Tweak comment. #
Total comments: 1
Patch Set 4 : Consider direct compositing reasons before skipping a chunk. #Patch Set 5 : Remove TODO comment. #
Total comments: 2
Patch Set 6 : Updated per discussion. #
Total comments: 7
Patch Set 7 : Feedback. #Patch Set 8 : Sync to head. #
Messages
Total messages: 49 (31 generated)
Description was changed from ========== Don't paint layers with tiny opacity in SPv2. BUG=713403 ========== to ========== Don't paint layers with tiny opacity in SPv2. BUG=713403 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
wkorman@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/2833883003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2833883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2769: bool PaintLayer::IsComposited() const { There is other logic in PaintLayer that checks GetCompositingState() and that perhaps we'd want to follow this method instead, but I thought best to keep this change focused on this one spot. I also considered adding a HasDirectCompositingReasons() method to ObjectPaintProperties to encapsulate the below but OPP is a very simple data model currently and so I hesitated to add such logic there for currently just this one call site, but open to thought. https://codereview.chromium.org/2833883003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp (left): https://codereview.chromium.org/2833883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp:37: ExpectPaintedOutputVisibility(element_name, expected_spv1, expected_spv1); With this patch we've seemingly achieved conformity between SPv1 and SPv2. I expected this to break some passing SPv2 animation tests but running locally I see only a small number of flaky failures/crashes which I believe is also the case at ToT. Am supportive of trying to land this and see how things go. https://codereview.chromium.org/2833883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp:40: void ExpectPaintedOutputVisibility(const char* element_name, I renamed this for easier mental parsing w.r.t. its functionality. https://codereview.chromium.org/2833883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp:798: ExpectPaintedOutputVisibility("target", true, false); Highlighting -- this is one of two test cases standardized by this patch. https://codereview.chromium.org/2833883003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp (right): https://codereview.chromium.org/2833883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp:859: ExpectPaintedOutputInvisible("target", false); Highlighting -- this is one of two test cases standardized by this patch.
The CQ bit was checked by wkorman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
wkorman@chromium.org changed reviewers: + pdr@chromium.org
+pdr as a fellow arborist.
This heuristic is looking a bit hacky. What if we moved this heuristic to the paint artifact compositor? That would not prevent painting, but it would prevent a composited layer and raster.
On 2017/04/21 21:55:46, chrishtr wrote: > This heuristic is looking a bit hacky. What if we moved this heuristic to > the paint artifact compositor? That would not prevent painting, but it would > prevent > a composited layer and raster. Seeking clarification -- are you proposing (1) doing away with PaintedOutputInvisible entirely, and moving the whole optimization to be SPv2-only and in PAC? Or, (2) leave current as-is, but for SPv2 implement the tiny-opacity optimization in PAC? I started implementing and realized I was doing #1, but perhaps you meant #2 since perhaps we don't want to lose this optimization in the meantime.
On 2017/04/22 at 00:06:49, wkorman wrote: > On 2017/04/21 21:55:46, chrishtr wrote: > > This heuristic is looking a bit hacky. What if we moved this heuristic to > > the paint artifact compositor? That would not prevent painting, but it would > > prevent > > a composited layer and raster. > > Seeking clarification -- are you proposing (1) doing away with PaintedOutputInvisible entirely, and moving the whole optimization to be SPv2-only and in PAC? Or, (2) leave current as-is, but for SPv2 implement the tiny-opacity optimization in PAC? > > I started implementing and realized I was doing #1, but perhaps you meant #2 since perhaps we don't want to lose this optimization in the meantime. I meant (1), yeah. Paint would always paint zero-opacity things, and PAC would dump content under zero-opacity or epsilon-opacity property tree effects.
Description was changed from ========== Don't paint layers with tiny opacity in SPv2. BUG=713403 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Skip paint chunks with effectively invisible opacity. Rework effectively invisible opacity optimization to achieve similar optimization with much simpler code in SPv2. Remove the optimization in SPv1 in favor of code simplicity in what is essentially a deprecated code path. More detail -- For SPv1 this change just removes the increasingly complex optimization edge cases needed to avoid painting content under various situations. For SPv2 we always paint, as in SPv1, but during layerization we scrutinize effect node and skip chunks that would be effectively invisible. This avoids the resource and processing overhead of creating, managing and rastering a layer for that chunk. BUG=713403 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
PTAL https://codereview.chromium.org/2833883003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayer.h (left): https://codereview.chromium.org/2833883003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.h:570: return IsTransparent() && After this change nothing seems to use the kPaintLayerHaveTransparency flag in PaintLayerFlags. We set it but never subsequently test it. Should we remove it in separate change? We might even be able to delete PaintsWithTransparency or at least centralize logic a bit, these two need to be reviewed: [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai... [2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai... https://codereview.chromium.org/2833883003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2833883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:511: // TODO(wkorman): Do we need to check blend mode, mask, or filter to skip Seeking your thought on this comment as I haven't thought deeply about it and plan to review once I see CQ dry run results. If we pass all tests we're probably ok as is.
The CQ bit was checked by wkorman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Skip paint chunks with effectively invisible opacity. Rework effectively invisible opacity optimization to achieve similar optimization with much simpler code in SPv2. Remove the optimization in SPv1 in favor of code simplicity in what is essentially a deprecated code path. More detail -- For SPv1 this change just removes the increasingly complex optimization edge cases needed to avoid painting content under various situations. For SPv2 we always paint, as in SPv1, but during layerization we scrutinize effect node and skip chunks that would be effectively invisible. This avoids the resource and processing overhead of creating, managing and rastering a layer for that chunk. BUG=713403 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Skip paint chunks with effectively invisible opacity. For SPv1 this change just removes the increasingly complex optimization edge cases needed to avoid painting content under various situations. For SPv2 we always paint, as in SPv1, but during layerization we scrutinize effect node and skip chunks that would be effectively invisible. This avoids the resource and processing overhead of creating, managing and rastering a layer for that chunk. BUG=713403 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Skip paint chunks with effectively invisible opacity. For SPv1 this change just removes the increasingly complex optimization edge cases needed to avoid painting content under various situations. For SPv2 we always paint, as in SPv1, but during layerization we scrutinize effect node and skip chunks that would be effectively invisible. This avoids the resource and processing overhead of creating, managing and rastering a layer for that chunk. BUG=713403 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Skip paint chunks with effectively invisible opacity. For SPv1 this change just removes the increasingly complex optimization edge cases needed to avoid painting content under permutations of minimum opacity and will-change hints. For SPv2 we always paint, as in SPv1, but during layerization we scrutinize effect node and skip chunks that would be effectively invisible. This avoids the resource and processing overhead of creating, managing and rastering a layer for that chunk. BUG=713403 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by wkorman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2833883003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (left): https://codereview.chromium.org/2833883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:67: bool PaintLayerPainter::PaintedOutputInvisible( I think we still need this for SPv1.
chrishtr@chromium.org changed reviewers: + trchen@chromium.org
https://codereview.chromium.org/2833883003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (left): https://codereview.chromium.org/2833883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:67: bool PaintLayerPainter::PaintedOutputInvisible( On 2017/04/26 19:58:21, chrishtr wrote: > I think we still need this for SPv1. Restored SPv1 behavior. https://codereview.chromium.org/2833883003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2833883003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:510: if (SkipEffectivelyInvisibleChunks(paint_artifact, current_group, chunk_it)) Revised per in person discussion with trchen@ around applying effectively invisible logic to child chunks. The cost of having to walk to find ancestor via StrictChildOfAlongPath for each is considered worthwhile as it allows us to avoid deemed-more-expensive compositor overhead.
Description was changed from ========== Skip paint chunks with effectively invisible opacity. For SPv1 this change just removes the increasingly complex optimization edge cases needed to avoid painting content under permutations of minimum opacity and will-change hints. For SPv2 we always paint, as in SPv1, but during layerization we scrutinize effect node and skip chunks that would be effectively invisible. This avoids the resource and processing overhead of creating, managing and rastering a layer for that chunk. BUG=713403 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Skip paint chunks with effectively invisible opacity. For SPv2 we always paint effectively invisible content, but during layerization we scrutinize effect node and skip chunks that would be effectively invisible. This simplifies code in SPv2 while still avoiding the resource and processing overhead of creating, managing and rastering a layer for that chunk. BUG=713403 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
https://codereview.chromium.org/2833883003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2833883003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:477: static bool SkipEffectivelyInvisibleChunks( I feel this could be confusing because it sounds like the helper inspects each chunk and skip the invisible ones. How about "SkipGroupIfEffectivelyInvisible"? https://codereview.chromium.org/2833883003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:487: !current_group.HasDirectCompositingReasons()) { nit: short branch first. i.e. if (!current_group.Opacity() < kMinimumVisibleOpacity || current_group.HasDirectCompositingReasons()) return false; https://codereview.chromium.org/2833883003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:489: // subgroup. I feel we should DCHECK the current chunk indeed belongs to the current group (which is guaranteed by caller thus we could skip the first chunk unconditionally), but it is a bit awkward to write. :/ #if DCHECK_IS_ON() { const EffectPaintPropertyNode* current_effect = chunk_it->properties.property_tree_state.Effect(); DCHECK(current_effect == ¤t_group || StrictChildOfAlongPath(¤t_group, current_effect)); } #endif
https://codereview.chromium.org/2833883003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp (right): https://codereview.chromium.org/2833883003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:477: static bool SkipEffectivelyInvisibleChunks( On 2017/04/27 00:24:10, trchen wrote: > I feel this could be confusing because it sounds like the helper inspects each > chunk and skip the invisible ones. How about "SkipGroupIfEffectivelyInvisible"? Done. https://codereview.chromium.org/2833883003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:487: !current_group.HasDirectCompositingReasons()) { On 2017/04/27 00:24:10, trchen wrote: > nit: short branch first. i.e. > if (!current_group.Opacity() < kMinimumVisibleOpacity || > current_group.HasDirectCompositingReasons()) > return false; Done. https://codereview.chromium.org/2833883003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:489: // subgroup. On 2017/04/27 00:24:10, trchen wrote: > I feel we should DCHECK the current chunk indeed belongs to the current group > (which is guaranteed by caller thus we could skip the first chunk > unconditionally), but it is a bit awkward to write. :/ > > #if DCHECK_IS_ON() > { > const EffectPaintPropertyNode* current_effect = > chunk_it->properties.property_tree_state.Effect(); > DCHECK(current_effect == ¤t_group || > StrictChildOfAlongPath(¤t_group, current_effect)); > } > #endif Done, I added a helper function that made this and below a shared single function call so we don't need the #if.
The CQ bit was checked by wkorman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
wkorman@chromium.org changed reviewers: + wangxianzhu@google.com - pdr@chromium.org
+wangxianzhu for OWNERS
core/paint lgtm.
schenney@chromium.org changed reviewers: + schenney@chromium.org
lgtm for platform. I wish we could somehow create a test that would fail if the backend supported more than 10 bit color. i.e. something that would start to fail when the underlying technology improves. But I realize we can't really do that.
chrishtr@chromium.org changed reviewers: + wangxianzhu@chromium.org
lgtm Sorry for the delay.
The CQ bit was checked by wkorman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, wangxianzhu@chromium.org, schenney@chromium.org, trchen@chromium.org Link to the patchset: https://codereview.chromium.org/2833883003/#ps140001 (title: "Sync to head.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493404270753640,
"parent_rev": "2e2708b5c1cb31e5d4dde1f105af1c473795d3b9", "commit_rev":
"caaa5c77ce09501d9dd409bde61ae226e1768b7f"}
Message was sent while issue was closed.
Description was changed from ========== Skip paint chunks with effectively invisible opacity. For SPv2 we always paint effectively invisible content, but during layerization we scrutinize effect node and skip chunks that would be effectively invisible. This simplifies code in SPv2 while still avoiding the resource and processing overhead of creating, managing and rastering a layer for that chunk. BUG=713403 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Skip paint chunks with effectively invisible opacity. For SPv2 we always paint effectively invisible content, but during layerization we scrutinize effect node and skip chunks that would be effectively invisible. This simplifies code in SPv2 while still avoiding the resource and processing overhead of creating, managing and rastering a layer for that chunk. BUG=713403 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2833883003 Cr-Commit-Position: refs/heads/master@{#468120} Committed: https://chromium.googlesource.com/chromium/src/+/caaa5c77ce09501d9dd409bde61a... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/caaa5c77ce09501d9dd409bde61a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
