Issue 183103007: Implement will-change-based GPU rasterization hint

Issue 183103007: Implement will-change-based GPU rasterization hint (Closed)

Created:
6 years, 9 months ago by ajuma

Modified:
6 years, 9 months ago

Reviewers:
Ian Vollick, jamesr, abarth-chromium

CC:
blink-reviews, mstensho+blink_opera.com, jamesr, krit, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, jbroman, blink-layers+watch_chromium.org, abarth-chromium, dglazkov+blink, Rik, jchaffraix+rendering, pdr., danakj, Stephen Chennney, rwlbuis, Vangelis Kokkevis, ernstm

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

Implement will-change-based GPU rasterization hint This uses an element's will-change value to determine whether it should have a GPU rasterization hint, and uses a newly-added acceleratedCompositingForGpuRasterizationHintEnabled setting to determine whether to create composited layers for elements that have a GPU rasterization hint. will-change is currently behind a runtime flag. Intent-to-implement: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/M62y2nKZ-gE Spec: http://tabatkins.github.io/specs/css-will-change/ BUG=313532 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169013

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 6

Patch Set 4 : Include will-change:background-color and will-change:background-position #

Patch Set 5 : Rebased #

Patch Set 6 : Move hint from WebLayer to WebContentLayer #

Created: 6 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+247 lines, -5 lines)			Patch
A	LayoutTests/compositing/will-change/gpu-rasterization-hint.html	View	1 2 3	1 chunk	+111 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/will-change/gpu-rasterization-hint-expected.txt	View	1 2 3	1 chunk	+68 lines, -0 lines	0 comments	Download
M	Source/core/frame/Settings.in	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/rendering/RenderBox.h	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderBoxModelObject.h	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderTableRow.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/compositing/CompositedLayerMapping.h	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/rendering/compositing/CompositedLayerMapping.cpp	View	1 2 3 4	3 chunks	+7 lines, -0 lines	0 comments	Download
M	Source/core/rendering/compositing/CompositingReasonFinder.cpp	View		1 chunk	+9 lines, -1 line	0 comments	Download
M	Source/core/rendering/style/RenderStyle.h	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/rendering/style/RenderStyle.cpp	View	1 2 3 4	2 chunks	+21 lines, -1 line	0 comments	Download
M	Source/platform/graphics/GraphicsLayer.h	View	1	2 chunks	+4 lines, -0 lines	0 comments	Download
M	Source/platform/graphics/GraphicsLayer.cpp	View	1 2 3 4 5	3 chunks	+12 lines, -0 lines	0 comments	Download
M	Source/web/WebSettingsImpl.h	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/web/WebSettingsImpl.cpp	View	1 2 3 4	1 chunk	+5 lines, -0 lines	0 comments	Download
M	public/platform/WebContentLayer.h	View	1 2 3 4 5	1 chunk	+2 lines, -0 lines	0 comments	Download
M	public/web/WebSettings.h	View		1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

Ian Vollick

On 2014/03/07 14:59:59, ajuma wrote: > PTAL. This depends on http://crrev.com/181063022. LayoutTests/ and Source/core/ LGTM ...

6 years, 9 months ago (2014-03-07 15:33:43 UTC) #2

Ian Vollick

https://codereview.chromium.org/183103007/diff/40001/LayoutTests/compositing/will-change/gpu-rasterization-hint.html File LayoutTests/compositing/will-change/gpu-rasterization-hint.html (right): https://codereview.chromium.org/183103007/diff/40001/LayoutTests/compositing/will-change/gpu-rasterization-hint.html#newcode80 LayoutTests/compositing/will-change/gpu-rasterization-hint.html:80: document.getElementById("layertree").innerText = window.internals.layerTreeAsText(document); Nit: it would be nice to ...

6 years, 9 months ago (2014-03-07 15:33:51 UTC) #3

abarth-chromium

https://codereview.chromium.org/183103007/diff/40001/Source/core/rendering/RenderBox.h File Source/core/rendering/RenderBox.h (right): https://codereview.chromium.org/183103007/diff/40001/Source/core/rendering/RenderBox.h#newcode74 Source/core/rendering/RenderBox.h:74: if (isRoot() || isPositioned() || createsGroup() || hasClipPath() || ...

6 years, 9 months ago (2014-03-07 16:04:38 UTC) #4

ajuma

6 years, 9 months ago (2014-03-07 17:03:41 UTC) #5

https://codereview.chromium.org/183103007/diff/40001/LayoutTests/compositing/...
File LayoutTests/compositing/will-change/gpu-rasterization-hint.html (right):

https://codereview.chromium.org/183103007/diff/40001/LayoutTests/compositing/...
LayoutTests/compositing/will-change/gpu-rasterization-hint.html:80:
document.getElementById("layertree").innerText =
window.internals.layerTreeAsText(document);
On 2014/03/07 15:33:52, Ian Vollick wrote:
> Nit: it would be nice to have a flag to optionally include the new layer tree
as
> text output.

This would be of limited value right now, since only tests which use
"will-change" will have the new output (so no existing tests are affected). If
we ever create gpu rasterization hints without will-change (e.g. for particular
types of content that seem good candidates for gpu rasterization), then a flag
might be more useful (depending on how many tests would need to be rebaselined
if we didn't have the flag). So I think we can defer having such a flag for now.

https://codereview.chromium.org/183103007/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/183103007/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.h:74: if (isRoot() || isPositioned() ||
createsGroup() || hasClipPath() || hasTransform() || hasHiddenBackface() ||
hasReflection() || style()->specifiesColumns() || !style()->hasAutoZIndex() ||
style()->hasWillChangeCompositingHint() ||
style()->hasWillChangeCompositingHint())
On 2014/03/07 16:04:39, abarth wrote:
> Why do we need this twice?  Maybe you meant hasWillChangeGpuRasterizationHint
?

Indeed, thanks for catching that!

https://codereview.chromium.org/183103007/diff/40001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/183103007/diff/40001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:863: case CSSPropertyHeight:
On 2014/03/07 15:33:52, Ian Vollick wrote:
> Dumb question: do we want background position/color to be in the list here,
too?

Good idea, I've added these to the list.

ajuma

+jamesr for Source/platform/, Source/web/, and public/ Note that this now depends on chromium CL http://crrev.com/192973002.

6 years, 9 months ago (2014-03-11 15:25:38 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/183103007/110001

6 years, 9 months ago (2014-03-12 13:01:19 UTC) #9

Message was sent while issue was closed.

Change committed as 169013

Expand Messages | Collapse Messages